From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 59A067260 for ; Sat, 13 Jan 2018 13:57:33 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Jan 2018 04:57:32 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,353,1511856000"; d="scan'208";a="10008809" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.252.9.53]) ([10.252.9.53]) by fmsmga002.fm.intel.com with ESMTP; 13 Jan 2018 04:57:30 -0800 To: Jianfeng Tan , dev@dpdk.org Cc: bruce.richardson@intel.com, konstantin.ananyev@intel.com, thomas@monjalon.net References: <1512067450-59203-1-git-send-email-jianfeng.tan@intel.com> <1515643654-129489-1-git-send-email-jianfeng.tan@intel.com> <1515643654-129489-2-git-send-email-jianfeng.tan@intel.com> From: "Burakov, Anatoly" Message-ID: <5ac110c8-2510-8046-2568-4a07ccd0f088@intel.com> Date: Sat, 13 Jan 2018 12:57:29 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <1515643654-129489-2-git-send-email-jianfeng.tan@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2 1/4] eal: add channel for multi-process communication X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 13 Jan 2018 12:57:33 -0000 On 11-Jan-18 4:07 AM, Jianfeng Tan wrote: > diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c > index 40fa982..d700e9e 100644 > + > int > rte_eal_primary_proc_alive(const char *config_file_path) > { > @@ -31,3 +75,347 @@ rte_eal_primary_proc_alive(const char *config_file_path) > > return !!ret; > } > + > +static struct action_entry * > +find_action_entry_by_name(const char *name) > +{ > + int len = strlen(name); Why do strlen() here? You already have MAX_ACTION_NAME_LEN, strncmp will take care of the rest, no? > + struct action_entry *entry; > + > + TAILQ_FOREACH(entry, &action_entry_list, next) { > + if (strncmp(entry->action_name, name, len) == 0) > + break; > + } > + > + return entry; > +} > + > +int > +rte_eal_mp_action_register(const char *action_name, rte_eal_mp_t action) > +{ > + struct action_entry *entry = malloc(sizeof(struct action_entry)); > + > + if (entry == NULL) { > + rte_errno = -ENOMEM; > + return -1; > + } > + > + if (strlen(action_name) > MAX_ACTION_NAME_LEN) { > + rte_errno = -E2BIG; > + return -1; > + } strnlen perhaps? strnlen(action_name) == MAX_ACTION_NAME_LEN will be an error condition, and unlike strlen you won't have to scan the entire memory if your string was corrupted. > + > + pthread_mutex_lock(&mp_mutex_action); > + if (find_action_entry_by_name(action_name) != NULL) { > + free(entry); > + rte_errno = -EEXIST; > + return -1; > + } > + strncpy(entry->action_name, action_name, MAX_ACTION_NAME_LEN); > + entry->action = action; > + TAILQ_INSERT_TAIL(&action_entry_list, entry, next); > + pthread_mutex_unlock(&mp_mutex_action); > + return 0; > +} > + > +void > +rte_eal_mp_action_unregister(const char *name) > +{ name == NULL? You do a find_action_entry_by_name with it, which calls strlen, which IIRC segfaults on NULL pointer. Also, maybe add an strlen (or better yet, strnlen) check here like in action_register, so that find_action_entry_by_name doesn't need to care about string lengths and can work off MAX_ACTION_NAME_LEN instead. > + struct action_entry *entry; > + > + pthread_mutex_lock(&mp_mutex_action); > + entry = find_action_entry_by_name(name); entry == NULL? > + TAILQ_REMOVE(&action_entry_list, entry, next); > + free(entry); > + pthread_mutex_unlock(&mp_mutex_action); > +} > + > +static int > +read_msg(int fd, char *buf, int buflen, int *fds, int fds_num) > +{ > + int ret; > + struct iovec iov; > + struct msghdr msgh; > + return -1; > + } > + > + params_len = len - sizeof(struct mp_msghdr); > + ret = entry->action(hdr->params, params_len, fds, hdr->fds_num); > + pthread_mutex_unlock(&mp_mutex_action); > + return ret; > + unnecessary newline. > +} > + > +static void * > +mp_handle(void *arg __rte_unused) > +{ > + int len; > + int fds[SCM_MAX_FD]; > + char buf[MAX_MSG_LENGTH]; > + > + while (1) { > + goto error; > + } > + > + return 0; > +error: > + close(mp_fd); > + mp_fd = -1; > + return -1; > +} > + > +static inline struct mp_msghdr * > +format_msg(const char *act_name, const void *p, int len_params, int fds_num) The name is slightly misleading, as this function actually *creates* a message, not just formats it. create_msg? alloc_msg? > +{ > + int len_msg; > + struct mp_msghdr *msg; > + > + len_msg = sizeof(struct mp_msghdr) + len_params; > + if (len_msg > MAX_MSG_LENGTH) { > + RTE_LOG(ERR, EAL, "Message is too long\n"); > + rte_errno = -EINVAL; > + return NULL; > + } > + > + msg = malloc(len_msg); > + if (!msg) { > + RTE_LOG(ERR, EAL, "Cannot alloc memory for msg\n"); > + rte_errno = -ENOMEM; > + return NULL; > + } > + memset(msg, 0, len_msg); > + strcpy(msg->action_name, act_name); strncpy? > + msg->fds_num = fds_num; > + msg->len_params = len_params; > + memcpy(msg->params, p, len_params); > + return msg; > +} > + > +static int > +send_msg(int fd, const char *dst_path, struct mp_msghdr *msg, int fds[]) > +{ > + int ret; > + struct msghdr msgh; > + struct iovec iov; > + size_t fd_size = msg->fds_num * sizeof(int); > + return mp_send(action_name, params, len_params, fds, fds_num); > +} > diff --git a/lib/librte_eal/common/eal_filesystem.h b/lib/librte_eal/common/eal_filesystem.h > index e8959eb..e95399b 100644 > --- a/lib/librte_eal/common/eal_filesystem.h > +++ b/lib/librte_eal/common/eal_filesystem.h > @@ -38,6 +38,23 @@ eal_runtime_config_path(void) > return buffer; > } > > +/** Path of primary/secondary communication unix socket file. */ > +#define MP_UNIX_PATH_FMT "%s/.%s_unix" > +static inline const char * > +eal_mp_unix_path(void) perhaps eal_mp_socket_path would've been more descriptive? API doesn't need to care what kind of socket it is. > +{ > + static char buffer[PATH_MAX]; /* static so auto-zeroed */ > + const char *directory = default_config_dir; > + const char *home_dir = getenv("HOME"); > + > + if (getuid() != 0 && home_dir != NULL) > + directory = home_dir; -- Thanks, Anatoly