Re: [dpdk-dev] [PATCH v2 1/4] eal: add channel for multi-process communication

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

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

<snip>

> +
>   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;

<snip>

> +		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) {

<snip>

> +		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);

<snip>

> +	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;

<snip>


-- 
Thanks,
Anatoly