From: Kamil Wiatrowski Date: Wed, 4 Sep 2019 12:39:25 +0000 (+0200) Subject: capabilities plugin: address review comments X-Git-Tag: collectd-5.11.0~17^2~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2c318297b805b8408a68040cf11349232f43b701;p=thirdparty%2Fcollectd.git capabilities plugin: address review comments Change-Id: I16b74ee6511ab1a1db8c7f3f0f515212286eb34e Signed-off-by: Kamil Wiatrowski --- diff --git a/src/capabilities.c b/src/capabilities.c index 29bba0433..5c27e6a3a 100644 --- a/src/capabilities.c +++ b/src/capabilities.c @@ -46,7 +46,7 @@ typedef struct dmi_type_name_s { const char *name; } dmi_type_name_t; -static char *g_cap_string = NULL; +static char *g_cap_json = NULL; static char *httpd_host = NULL; static unsigned short httpd_port = 9104; static struct MHD_Daemon *httpd; @@ -127,13 +127,13 @@ static int cap_get_dmi_variables(json_t *parent, const dmi_type type, DEBUG(" %s:%s", reader.name, reader.value); attributes = NULL; if (entries == NULL) { - ERROR(CAP_PLUGIN ": unexpected dmi output format"); + ERROR(CAP_PLUGIN ": unexpected dmi output format."); dmi_reader_clean(&reader); return -1; } if (json_object_set_new(entries, reader.name, json_string(reader.value))) { - ERROR(CAP_PLUGIN ": Failed to set json entry."); + ERROR(CAP_PLUGIN ": Failed to set json object for entries."); dmi_reader_clean(&reader); return -1; } @@ -142,7 +142,7 @@ static int cap_get_dmi_variables(json_t *parent, const dmi_type type, case DMI_ENTRY_LIST_NAME: DEBUG(" %s:", reader.name); if (entries == NULL) { - ERROR(CAP_PLUGIN ": unexpected dmi output format"); + ERROR(CAP_PLUGIN ": unexpected dmi output format."); dmi_reader_clean(&reader); return -1; } @@ -153,7 +153,8 @@ static int cap_get_dmi_variables(json_t *parent, const dmi_type type, return -1; } if (json_object_set_new(entries, reader.name, attributes)) { - ERROR(CAP_PLUGIN ": Failed to set json entry."); + ERROR(CAP_PLUGIN ": Failed to set json object for entry %s.", + reader.name); dmi_reader_clean(&reader); return -1; } @@ -202,15 +203,19 @@ static int cap_http_handler(void *cls, struct MHD_Connection *connection, *connection_state = &(int){44}; return MHD_YES; } - DEBUG(CAP_PLUGIN ": formatted response: %s", g_cap_string); + DEBUG(CAP_PLUGIN ": formatted response: %s", g_cap_json); #if defined(MHD_VERSION) && MHD_VERSION >= 0x00090500 struct MHD_Response *res = MHD_create_response_from_buffer( - strlen(g_cap_string), g_cap_string, MHD_RESPMEM_PERSISTENT); + strlen(g_cap_json), g_cap_json, MHD_RESPMEM_PERSISTENT); #else struct MHD_Response *res = - MHD_create_response_from_data(strlen(g_cap_string), g_cap_string, 0, 0); + MHD_create_response_from_data(strlen(g_cap_json), g_cap_json, 0, 0); #endif + if (res == NULL) { + ERROR(CAP_PLUGIN ": MHD create response failed."); + return MHD_NO; + } MHD_add_response_header(res, MHD_HTTP_HEADER_CONTENT_TYPE, CONTENT_TYPE_JSON); int status = MHD_queue_response(connection, MHD_HTTP_OK, res); @@ -264,12 +269,14 @@ static int cap_open_socket() { } if (bind(fd, ai->ai_addr, ai->ai_addrlen) != 0) { + INFO(CAP_PLUGIN ": bind failed: %s", STRERRNO); close(fd); fd = -1; continue; } if (listen(fd, LISTEN_BACKLOG) != 0) { + INFO(CAP_PLUGIN ": listen failed: %s", STRERRNO); close(fd); fd = -1; continue; @@ -313,10 +320,8 @@ static struct MHD_Daemon *cap_start_daemon() { #endif MHD_OPTION_EXTERNAL_LOGGER, cap_logger, NULL, MHD_OPTION_END); - if (d == NULL) { + if (d == NULL) ERROR(CAP_PLUGIN ": MHD_start_daemon() failed."); - return NULL; - } return d; } @@ -364,7 +369,7 @@ static int cap_shutdown() { } sfree(httpd_host); - sfree(g_cap_string); + sfree(g_cap_json); return 0; } @@ -383,10 +388,10 @@ static int cap_init(void) { return -1; } - g_cap_string = json_dumps(root, JSON_COMPACT); + g_cap_json = json_dumps(root, JSON_COMPACT); json_decref(root); - if (g_cap_string == NULL) { + if (g_cap_json == NULL) { ERROR(CAP_PLUGIN ": json_dumps() failed."); cap_shutdown(); return -1; diff --git a/src/capabilities_test.c b/src/capabilities_test.c index 95ac66657..2d9edd42a 100644 --- a/src/capabilities_test.c +++ b/src/capabilities_test.c @@ -52,6 +52,8 @@ static entry_type entry[] = { DMI_ENTRY_MAP, DMI_ENTRY_END}; static size_t len = STATIC_ARRAY_SIZE(entry); +static struct MHD_Response *mhd_res = NULL; + /* mock functions */ int dmi_reader_init(dmi_reader_t *reader, const dmi_type type) { reader->current_type = DMI_ENTRY_NONE; @@ -82,13 +84,13 @@ void MHD_stop_daemon(struct MHD_Daemon *daemon) {} struct MHD_Response * MHD_create_response_from_buffer(size_t size, void *data, enum MHD_ResponseMemoryMode mode) { - return NULL; + return mhd_res; } struct MHD_Response *MHD_create_response_from_data(size_t size, void *data, int must_free, int must_copy) { - return NULL; + return mhd_res; } int MHD_add_response_header(struct MHD_Response *response, const char *header, @@ -212,7 +214,7 @@ DEF_TEST(plugin_config_fail) { DEF_TEST(http_handler) { void *state = NULL; - g_cap_string = "TEST"; + g_cap_json = "TEST"; int ret = cap_http_handler(NULL, NULL, NULL, MHD_HTTP_METHOD_PUT, NULL, NULL, NULL, &state); EXPECT_EQ_INT(MHD_NO, ret); @@ -223,12 +225,20 @@ DEF_TEST(http_handler) { EXPECT_EQ_INT(MHD_YES, ret); CHECK_NOT_NULL(state); + ret = cap_http_handler(NULL, NULL, NULL, MHD_HTTP_METHOD_GET, NULL, NULL, + NULL, &state); + EXPECT_EQ_INT(MHD_NO, ret); + CHECK_NOT_NULL(state); + + /* mock not NULL pointer */ + mhd_res = (struct MHD_Response *)&(int){0}; ret = cap_http_handler(NULL, NULL, NULL, MHD_HTTP_METHOD_GET, NULL, NULL, NULL, &state); EXPECT_EQ_INT(MHD_HTTP_OK, ret); CHECK_NOT_NULL(state); - g_cap_string = NULL; + g_cap_json = NULL; + mhd_res = NULL; return 0; } diff --git a/src/collectd.conf.pod b/src/collectd.conf.pod index 92d6e3e4a..f676abb3e 100644 --- a/src/collectd.conf.pod +++ b/src/collectd.conf.pod @@ -1478,7 +1478,7 @@ returned by plugin is in json format. B - Host "" + Host "localhost" Port "9104" diff --git a/src/utils/dmi/dmi.c b/src/utils/dmi/dmi.c index 12e98845a..5fe382390 100644 --- a/src/utils/dmi/dmi.c +++ b/src/utils/dmi/dmi.c @@ -53,7 +53,7 @@ #include "utils/common/common.h" #include "utils/dmi/dmi.h" -#define UTIL_NAME "DMI_READER" +#define UTIL_NAME "dmi_reader" #define DMIDECODE_CMD_FMT_LEN DMI_MAX_LEN @@ -66,7 +66,7 @@ static int dmi_read_entry(dmi_reader_t *r) { buff++; if (*buff == '\0') { r->current_type = DMI_ENTRY_NONE; - r->_read_callback = dmi_look_for_handle; + r->_read_next = dmi_look_for_handle; return DMI_OK; } @@ -108,7 +108,7 @@ static int dmi_read_type_name(dmi_reader_t *r) { r->name = r->_buff; r->current_type = DMI_ENTRY_NAME; - r->_read_callback = dmi_read_entry; + r->_read_next = dmi_read_entry; return DMI_OK; } @@ -123,7 +123,7 @@ static int dmi_look_for_handle(dmi_reader_t *r) { if (strncmp(handle, r->_buff, strlen(handle)) != 0) continue; - r->_read_callback = dmi_read_type_name; + r->_read_next = dmi_read_type_name; return DMI_OK; } @@ -153,8 +153,8 @@ int dmi_reader_init(dmi_reader_t *reader, const dmi_type type) { reader->name = NULL; reader->value = NULL; - reader->_read_callback = dmi_look_for_handle; reader->current_type = DMI_ENTRY_NONE; + reader->_read_next = dmi_look_for_handle; return DMI_OK; } @@ -165,23 +165,25 @@ void dmi_reader_clean(dmi_reader_t *reader) { return; } - if (reader->_fd) + if (reader->_fd) { pclose(reader->_fd); + reader->_fd = NULL; + } - reader->_fd = NULL; - reader->_read_callback = NULL; + reader->_read_next = NULL; } int dmi_read_next(dmi_reader_t *reader) { - if (reader == NULL || reader->_read_callback == NULL || reader->_fd == NULL) { + if (reader == NULL || reader->_read_next == NULL || reader->_fd == NULL) { ERROR(UTIL_NAME ".%s: NULL pointer.", __func__); return DMI_ERROR; } - int ret = reader->_read_callback(reader); + int ret = reader->_read_next(reader); if (reader->current_type == DMI_ENTRY_END || ret == DMI_ERROR) { pclose(reader->_fd); - reader->_read_callback = NULL; + reader->_fd = NULL; + reader->_read_next = NULL; DEBUG(UTIL_NAME ": dmidecode reader finished, status=%d.", ret); } diff --git a/src/utils/dmi/dmi.h b/src/utils/dmi/dmi.h index 181477f90..c81ff77b6 100644 --- a/src/utils/dmi/dmi.h +++ b/src/utils/dmi/dmi.h @@ -95,7 +95,7 @@ typedef enum entry_type_e { typedef struct dmi_reader_s { FILE *_fd; char _buff[DMI_MAX_LEN]; - int (*_read_callback)(struct dmi_reader_s *reader); + int (*_read_next)(struct dmi_reader_s *reader); /* Type of current entry */ entry_type current_type; /* Entry name, the pointer changes after every read. */