From e7eb2fbed20a051cd1610635f4ac921be9a1040a Mon Sep 17 00:00:00 2001 From: Krzysztof Matczak Date: Thu, 22 Dec 2016 12:29:32 +0000 Subject: [PATCH] Addressing PR comments related with dpdk_utils Change-Id: I4d6e132e0b5aa940a9c444c141967c8b79d90f0e Signed-off-by: Krzysztof Matczak --- src/utils_dpdk.c | 151 ++++++++++++++++++++++++------------------------------- src/utils_dpdk.h | 9 ---- 2 files changed, 66 insertions(+), 94 deletions(-) diff --git a/src/utils_dpdk.c b/src/utils_dpdk.c index 51aa91ac..640f08bc 100644 --- a/src/utils_dpdk.c +++ b/src/utils_dpdk.c @@ -57,9 +57,7 @@ enum DPDK_HELPER_STATUS { }; #define DPDK_HELPER_TRACE(_name) \ - DEBUG("%s:%s:%d pid=%lu", _name, __FUNCTION__, __LINE__, (long)getpid()) - -#define DPDK_HELPER_USE_PIPES + DEBUG("%s:%s:%d pid=%ld", _name, __FUNCTION__, __LINE__, (long)getpid()) struct dpdk_helper_ctx_s { @@ -74,9 +72,7 @@ struct dpdk_helper_ctx_s { cdtime_t cmd_wait_time; pid_t pid; -#ifdef DPDK_HELPER_USE_PIPES int pipes[2]; -#endif /* DPDK_HELPER_USE_PIPES */ int status; int cmd; @@ -86,7 +82,7 @@ struct dpdk_helper_ctx_s { }; static int dpdk_shm_init(const char *name, size_t size, void **map); -static int dpdk_shm_cleanup(const char *name, size_t size, void *map); +static void dpdk_shm_cleanup(const char *name, size_t size, void *map); static int dpdk_helper_spawn(dpdk_helper_ctx_t *phc); static int dpdk_helper_worker(dpdk_helper_ctx_t *phc); @@ -126,7 +122,7 @@ int dpdk_helper_eal_config_set(dpdk_helper_ctx_t *phc, dpdk_eal_config_t *ec) { return -EINVAL; } - memcpy(&phc->eal_config, ec, sizeof(dpdk_eal_config_t)); + memcpy(&phc->eal_config, ec, sizeof(phc->eal_config)); return 0; } @@ -144,7 +140,7 @@ int dpdk_helper_eal_config_get(dpdk_helper_ctx_t *phc, dpdk_eal_config_t *ec) { return -EINVAL; } - memcpy(ec, &phc->eal_config, sizeof(dpdk_eal_config_t)); + memcpy(ec, &phc->eal_config, sizeof(*ec)); return 0; } @@ -162,34 +158,44 @@ int dpdk_helper_eal_config_parse(dpdk_helper_ctx_t *phc, oconfig_item_t *ci) { return -EINVAL; } + int status = 0; for (int i = 0; i < ci->children_num; i++) { oconfig_item_t *child = ci->children + i; if (strcasecmp("Coremask", child->key) == 0) { - cf_util_get_string_buffer(child, phc->eal_config.coremask, - sizeof(phc->eal_config.coremask)); + status = cf_util_get_string_buffer(child, phc->eal_config.coremask, + sizeof(phc->eal_config.coremask)); DEBUG("dpdk_common: EAL:Coremask %s", phc->eal_config.coremask); } else if (strcasecmp("MemoryChannels", child->key) == 0) { - cf_util_get_string_buffer(child, phc->eal_config.memory_channels, - sizeof(phc->eal_config.memory_channels)); + status = + cf_util_get_string_buffer(child, phc->eal_config.memory_channels, + sizeof(phc->eal_config.memory_channels)); DEBUG("dpdk_common: EAL:Memory Channels %s", phc->eal_config.memory_channels); } else if (strcasecmp("SocketMemory", child->key) == 0) { - cf_util_get_string_buffer(child, phc->eal_config.socket_memory, - sizeof(phc->eal_config.socket_memory)); + status = cf_util_get_string_buffer(child, phc->eal_config.socket_memory, + sizeof(phc->eal_config.socket_memory)); DEBUG("dpdk_common: EAL:Socket memory %s", phc->eal_config.socket_memory); } else if (strcasecmp("ProcessType", child->key) == 0) { - cf_util_get_string_buffer(child, phc->eal_config.process_type, - sizeof(phc->eal_config.process_type)); + status = cf_util_get_string_buffer(child, phc->eal_config.process_type, + sizeof(phc->eal_config.process_type)); DEBUG("dpdk_common: EAL:Process type %s", phc->eal_config.process_type); } else if ((strcasecmp("FilePrefix", child->key) == 0) && (child->values[0].type == OCONFIG_TYPE_STRING)) { ssnprintf(phc->eal_config.file_prefix, DATA_MAX_NAME_LEN, "/var/run/.%s_config", child->values[0].value.string); DEBUG("dpdk_common: EAL:File prefix %s", phc->eal_config.file_prefix); + } else { + ERROR("dpdk_common: Invalid '%s' configuration option", child->key); + status = -EINVAL; + } + + if (status != 0) { + ERROR("dpdk_common: Parsing EAL configuration failed"); + break; } } - return 0; + return status; } static int dpdk_shm_init(const char *name, size_t size, void **map) { @@ -199,60 +205,54 @@ static int dpdk_shm_init(const char *name, size_t size, void **map) { int fd = shm_open(name, O_CREAT | O_TRUNC | O_RDWR, 0666); if (fd < 0) { - WARNING("dpdk_shm_init: Failed to open %s as SHM:%s\n", name, + WARNING("dpdk_shm_init: Failed to open %s as SHM:%s", name, sstrerror(errno, errbuf, sizeof(errbuf))); - goto fail; + *map = NULL; + return -1; } int ret = ftruncate(fd, size); if (ret != 0) { - WARNING("dpdk_shm_init: Failed to resize SHM:%s\n", + WARNING("dpdk_shm_init: Failed to resize SHM:%s", sstrerror(errno, errbuf, sizeof(errbuf))); - goto fail_close; + close(fd); + *map = NULL; + dpdk_shm_cleanup(name, size, NULL); + return -1; } *map = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); if (*map == MAP_FAILED) { - WARNING("dpdk_shm_init:Failed to mmap SHM:%s\n", + WARNING("dpdk_shm_init:Failed to mmap SHM:%s", sstrerror(errno, errbuf, sizeof(errbuf))); - goto fail_close; + close(fd); + *map = NULL; + dpdk_shm_cleanup(name, size, NULL); + return -1; } - /* - * Close the file descriptor, the shared memory object still exists - * and can only be removed by calling shm_unlink(). - */ + /* File descriptor no longer needed for shared memory operations */ close(fd); - memset(*map, 0, size); return 0; - -fail_close: - close(fd); -fail: - *map = NULL; - return -1; } -static int dpdk_shm_cleanup(const char *name, size_t size, void *map) { +static void dpdk_shm_cleanup(const char *name, size_t size, void *map) { DPDK_HELPER_TRACE(name); + char errbuf[ERR_BUF_SIZE]; - int ret = munmap(map, size); - if (ret) { - ERROR("munmap returned %d\n", ret); - } - - ret = shm_unlink(name); - if (ret) { - ERROR("shm_unlink returned %d\n", ret); + if (map != NULL) { + if (munmap(map, size)) + ERROR("munmap failure %s", sstrerror(errno, errbuf, sizeof(errbuf))); } - return 0; + if (shm_unlink(name)) + ERROR("shm_unlink failure %s", sstrerror(errno, errbuf, sizeof(errbuf))); } -inline void *dpdk_helper_priv_get(dpdk_helper_ctx_t *phc) { +void *dpdk_helper_priv_get(dpdk_helper_ctx_t *phc) { if (phc) - return (void *)phc->priv_data; + return phc->priv_data; return NULL; } @@ -268,7 +268,6 @@ int dpdk_helper_data_size_get(dpdk_helper_ctx_t *phc) { int dpdk_helper_init(const char *name, size_t data_size, dpdk_helper_ctx_t **pphc) { - int err = 0; dpdk_helper_ctx_t *phc = NULL; size_t shm_size = sizeof(dpdk_helper_ctx_t) + data_size; char errbuf[ERR_BUF_SIZE]; @@ -288,26 +287,28 @@ int dpdk_helper_init(const char *name, size_t data_size, /* Allocate dpdk_helper_ctx_t and * initialize a POSIX SHared Memory (SHM) object. */ - err = dpdk_shm_init(name, shm_size, (void **)&phc); + int err = dpdk_shm_init(name, shm_size, (void **)&phc); if (err != 0) { return -errno; } err = sem_init(&phc->sema_cmd_start, 1, 0); if (err != 0) { - ERROR("sema_cmd_start semaphore init failed: %s\n", + ERROR("sema_cmd_start semaphore init failed: %s", sstrerror(errno, errbuf, sizeof(errbuf))); + int errno_m = errno; dpdk_shm_cleanup(name, shm_size, (void *)phc); - return -errno; + return -errno_m; } err = sem_init(&phc->sema_cmd_complete, 1, 0); if (err != 0) { - ERROR("sema_cmd_complete semaphore init failed: %s\n", + ERROR("sema_cmd_complete semaphore init failed: %s", sstrerror(errno, errbuf, sizeof(errbuf))); sem_destroy(&phc->sema_cmd_start); + int errno_m = errno; dpdk_shm_cleanup(name, shm_size, (void *)phc); - return -errno; + return -errno_m; } phc->shm_size = shm_size; @@ -328,9 +329,7 @@ int dpdk_helper_shutdown(dpdk_helper_ctx_t *phc) { DPDK_HELPER_TRACE(phc->shm_name); -#ifdef DPDK_HELPER_USE_PIPES close(phc->pipes[1]); -#endif if (phc->status != DPDK_HELPER_NOT_INITIALIZED) { dpdk_helper_exit_command(phc, DPDK_HELPER_GRACEFUL_QUIT); @@ -355,7 +354,6 @@ static int dpdk_helper_spawn(dpdk_helper_ctx_t *phc) { phc->eal_initialized = 0; phc->cmd_wait_time = MS_TO_CDTIME_T(DPDK_CDM_DEFAULT_TIMEOUT); -#ifdef DPDK_HELPER_USE_PIPES /* * Create a pipe for helper stdout back to collectd. This is necessary for * logging EAL failures, as rte_eal_init() calls rte_panic(). @@ -368,7 +366,7 @@ static int dpdk_helper_spawn(dpdk_helper_ctx_t *phc) { } if (pipe(phc->pipes) != 0) { - DEBUG("dpdk_helper_spawn: Could not create helper pipe: %s\n", + DEBUG("dpdk_helper_spawn: Could not create helper pipe: %s", sstrerror(errno, errbuf, sizeof(errbuf))); return -1; } @@ -385,28 +383,23 @@ static int dpdk_helper_spawn(dpdk_helper_ctx_t *phc) { WARNING("dpdk_helper_spawn: error setting up pipes: %s", sstrerror(errno, errbuf, sizeof(errbuf))); } -#endif /* DPDK_HELPER_USE_PIPES */ pid_t pid = fork(); if (pid > 0) { phc->pid = pid; -#ifdef DPDK_HELPER_USE_PIPES close(phc->pipes[1]); -#endif /* DPDK_HELPER_USE_PIPES */ DEBUG("%s:dpdk_helper_spawn: helper pid %lu", phc->shm_name, (long)phc->pid); } else if (pid == 0) { -#ifdef DPDK_HELPER_USE_PIPES /* Replace stdout with a pipe to collectd. */ close(phc->pipes[0]); close(STDOUT_FILENO); dup2(phc->pipes[1], STDOUT_FILENO); -#endif /* DPDK_HELPER_USE_PIPES */ DPDK_CHILD_TRACE(phc->shm_name); dpdk_helper_worker(phc); exit(0); } else { - ERROR("dpdk_helper_start: Failed to fork helper process: %s\n", + ERROR("dpdk_helper_start: Failed to fork helper process: %s", sstrerror(errno, errbuf, sizeof(errbuf))); return -1; } @@ -419,9 +412,7 @@ static int dpdk_helper_exit(dpdk_helper_ctx_t *phc, DPDK_CHILD_LOG("%s:%s:%d %s\n", phc->shm_name, __FUNCTION__, __LINE__, dpdk_helper_status_str(status)); -#ifdef DPDK_HELPER_USE_PIPES close(phc->pipes[1]); -#endif /* DPDK_HELPER_USE_PIPES */ phc->status = status; @@ -435,9 +426,7 @@ static int dpdk_helper_exit_command(dpdk_helper_ctx_t *phc, char errbuf[ERR_BUF_SIZE]; DPDK_HELPER_TRACE(phc->shm_name); -#ifdef DPDK_HELPER_USE_PIPES close(phc->pipes[1]); -#endif /* DPDK_HELPER_USE_PIPES */ if (phc->status == DPDK_HELPER_ALIVE_SENDING_EVENTS) { phc->status = status; @@ -451,7 +440,7 @@ static int dpdk_helper_exit_command(dpdk_helper_ctx_t *phc, int err = kill(phc->pid, SIGKILL); if (err) { - ERROR("%s error sending kill to helper: %s\n", __FUNCTION__, + ERROR("%s error sending kill to helper: %s", __FUNCTION__, sstrerror(errno, errbuf, sizeof(errbuf))); } } @@ -462,7 +451,7 @@ static int dpdk_helper_exit_command(dpdk_helper_ctx_t *phc, int err = kill(phc->pid, SIGKILL); if (err) { - ERROR("%s error sending kill to helper: %s\n", __FUNCTION__, + ERROR("%s error sending kill to helper: %s", __FUNCTION__, sstrerror(errno, errbuf, sizeof(errbuf))); } } @@ -700,7 +689,6 @@ static int dpdk_helper_status_check(dpdk_helper_ctx_t *phc) { return 0; } -#ifdef DPDK_HELPER_USE_PIPES static void dpdk_helper_check_pipe(dpdk_helper_ctx_t *phc) { char buf[DPDK_MAX_BUFFER_SIZE]; char out[DPDK_MAX_BUFFER_SIZE]; @@ -725,7 +713,6 @@ static void dpdk_helper_check_pipe(dpdk_helper_ctx_t *phc) { DEBUG("%s: helper process:\n%s", phc->shm_name, out); } } -#endif /* DPDK_HELPER_USE_PIPES */ int dpdk_helper_command(dpdk_helper_ctx_t *phc, enum DPDK_CMD cmd, int *result, cdtime_t cmd_wait_time) { @@ -739,13 +726,9 @@ int dpdk_helper_command(dpdk_helper_ctx_t *phc, enum DPDK_CMD cmd, int *result, phc->cmd_wait_time = cmd_wait_time; - int ret = 0; + int ret = dpdk_helper_status_check(phc); - ret = dpdk_helper_status_check(phc); - -#ifdef DPDK_HELPER_USE_PIPES dpdk_helper_check_pipe(phc); -#endif /* DPDK_HELPER_USE_PIPES */ if (ret != 0) { return ret; @@ -760,7 +743,7 @@ int dpdk_helper_command(dpdk_helper_ctx_t *phc, enum DPDK_CMD cmd, int *result, int err = sem_post(&phc->sema_cmd_start); if (err) { char errbuf[ERR_BUF_SIZE]; - ERROR("dpdk_helper_worker: error posting sema_cmd_start semaphore (%s)\n", + ERROR("dpdk_helper_worker: error posting sema_cmd_start semaphore (%s)", sstrerror(errno, errbuf, sizeof(errbuf))); } @@ -803,9 +786,7 @@ int dpdk_helper_command(dpdk_helper_ctx_t *phc, enum DPDK_CMD cmd, int *result, } } -#ifdef DPDK_HELPER_USE_PIPES dpdk_helper_check_pipe(phc); -#endif /* DPDK_HELPER_USE_PIPES */ DEBUG("%s: DPDK command complete (cmd=%d, result=%d)", phc->shm_name, phc->cmd, phc->cmd_result); @@ -833,18 +814,20 @@ uint128_t str_to_uint128(const char *str, int len) { uint128_t lcore_mask; int err = 0; - memset(&lcore_mask, 0, sizeof(uint128_t)); + memset(&lcore_mask, 0, sizeof(lcore_mask)); if (len <= 2 || strncmp(str, "0x", 2) != 0) { ERROR("%s Value %s should be represened in hexadecimal format", __FUNCTION__, str); return lcore_mask; } - + /* If str is <= 64 bit long ('0x' + 16 chars = 18 chars) then + * conversion is straightforward. Otherwise str is splitted into 64b long + * blocks */ if (len <= 18) { lcore_mask.low = strtoull_safe(str, &err); if (err) - goto parse_out; + return lcore_mask; } else { char low_str[DATA_MAX_NAME_LEN]; char high_str[DATA_MAX_NAME_LEN]; @@ -857,15 +840,13 @@ uint128_t str_to_uint128(const char *str, int len) { lcore_mask.low = strtoull_safe(low_str, &err); if (err) - goto parse_out; + return lcore_mask; lcore_mask.high = strtoull_safe(high_str, &err); if (err) { lcore_mask.low = 0; - goto parse_out; + return lcore_mask; } } - -parse_out: return lcore_mask; } diff --git a/src/utils_dpdk.h b/src/utils_dpdk.h index a4731d03..c9bb14b0 100644 --- a/src/utils_dpdk.h +++ b/src/utils_dpdk.h @@ -37,15 +37,6 @@ #define ERR_BUF_SIZE 1024 -#if RTE_VER_RELEASE == 16 && RTE_VER_MINOR == 0 -#if RTE_VER_MONTH == 4 -#define DPDK_VER_16_04 RTE_VERSION_NUM(16, 4, 0, 16) -#endif -#if RTE_VER_MONTH == 7 -#define DPDK_VER_16_07 RTE_VERSION_NUM(16, 7, 0, 16) -#endif -#endif - enum DPDK_CMD { DPDK_CMD_NONE = 0, DPDK_CMD_QUIT, -- 2.11.0