Re: [PATCH v2] genl: Add unicast listener

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8426 bytes --]

Hi Tim,

On 06/24/2016 03:13 PM, Tim Kourt wrote:
> ---
>   ell/genl.c | 113 ++++++++++++++++++++++++++++++++++++++-----------------------
>   ell/genl.h |  10 ++++--
>   2 files changed, 78 insertions(+), 45 deletions(-)
>
> diff --git a/ell/genl.c b/ell/genl.c
> index 15f4782..767bd2f 100644
> --- a/ell/genl.c
> +++ b/ell/genl.c
> @@ -25,7 +25,6 @@
>   #endif
>
>   #include <errno.h>
> -#include <unistd.h>
>   #include <sys/socket.h>
>   #include <linux/genetlink.h>
>
> @@ -172,7 +171,9 @@ static void mcast_free(void *data, void *user_data)
>   	if (genl && mcast->users > 0) {
>   		int group = mcast->id;
>
> -		setsockopt(genl->fd, SOL_NETLINK, NETLINK_DROP_MEMBERSHIP,
> +		if (group)
> +			setsockopt(genl->fd, SOL_NETLINK,
> +							NETLINK_DROP_MEMBERSHIP,
>   							&group, sizeof(group));
>   	}
>
> @@ -227,7 +228,7 @@ static void family_add_mcast(struct l_genl_family *family, const char *name,
>
>   	strncpy(mcast->name, name, GENL_NAMSIZ);
>   	mcast->id = id;
> -	mcast->users = 0;
> +	mcast->users = id ? 0 : 1; /* id = 0 --> unicast */
>
>   	l_queue_push_tail(family->mcast_list, mcast);
>   }
> @@ -356,44 +357,6 @@ static bool match_request_seq(const void *a, const void *b)
>   	return request->seq == seq;
>   }
>
> -static void process_request(struct l_genl *genl, const struct nlmsghdr *nlmsg)
> -{
> -	struct l_genl_msg *msg;
> -	struct genl_request *request;
> -
> -	if (nlmsg->nlmsg_type == NLMSG_NOOP ||
> -					nlmsg->nlmsg_type == NLMSG_OVERRUN)
> -		return;
> -
> -	request = l_queue_remove_if(genl->pending_list, match_request_seq,
> -					L_UINT_TO_PTR(nlmsg->nlmsg_seq));
> -	if (!request)
> -		return;
> -
> -	msg = _genl_msg_create(nlmsg);
> -	if (!msg) {
> -		destroy_request(request);
> -		wakeup_writer(genl);
> -		return;
> -	}
> -
> -	if (request->callback && nlmsg->nlmsg_type != NLMSG_DONE)
> -		request->callback(msg, request->user_data);
> -
> -	if (nlmsg->nlmsg_flags & NLM_F_MULTI) {
> -		if (nlmsg->nlmsg_type == NLMSG_DONE) {
> -			destroy_request(request);
> -			wakeup_writer(genl);
> -		} else
> -			l_queue_push_head(genl->pending_list, request);
> -	} else {
> -		destroy_request(request);
> -		wakeup_writer(genl);
> -	}
> -
> -	l_genl_msg_unref(msg);
> -}
> -
>   struct notify_type_group {
>   	struct l_genl_msg *msg;
>   	uint16_t type;
> @@ -432,6 +395,46 @@ static void process_notify(struct l_genl *genl, uint32_t group,
>   	l_genl_msg_unref(match.msg);
>   }
>
> +static void process_request(struct l_genl *genl, const struct nlmsghdr *nlmsg)
> +{
> +	struct l_genl_msg *msg;
> +	struct genl_request *request;
> +
> +	if (nlmsg->nlmsg_type == NLMSG_NOOP ||
> +					nlmsg->nlmsg_type == NLMSG_OVERRUN)
> +		return;
> +
> +	request = l_queue_remove_if(genl->pending_list, match_request_seq,
> +					L_UINT_TO_PTR(nlmsg->nlmsg_seq));
> +	if (!request) {
> +		process_notify(genl, 0, nlmsg);
> +		return;

So inside received_data we do:

if (group > 0)
	process_notify
else
	process_request

And here we call process_notify().  This is weird.  Is there another 
selector besides group we can use to identify the message as a request 
from the kernel?

> +	}
> +
> +	msg = _genl_msg_create(nlmsg);
> +	if (!msg) {
> +		destroy_request(request);
> +		wakeup_writer(genl);
> +		return;
> +	}
> +
> +	if (request->callback && nlmsg->nlmsg_type != NLMSG_DONE)
> +		request->callback(msg, request->user_data);
> +
> +	if (nlmsg->nlmsg_flags & NLM_F_MULTI) {
> +		if (nlmsg->nlmsg_type == NLMSG_DONE) {
> +			destroy_request(request);
> +			wakeup_writer(genl);
> +		} else
> +			l_queue_push_head(genl->pending_list, request);
> +	} else {
> +		destroy_request(request);
> +		wakeup_writer(genl);
> +	}
> +
> +	l_genl_msg_unref(msg);
> +}
> +
>   static void read_watch_destroy(void *user_data)
>   {
>   }
> @@ -528,7 +531,7 @@ LIB_EXPORT struct l_genl *l_genl_new(int fd)
>   	return l_genl_ref(genl);
>   }
>
> -LIB_EXPORT struct l_genl *l_genl_new_default(void)
> +LIB_EXPORT struct l_genl *l_genl_new_default_for_pid(pid_t pid)
>   {
>   	struct l_genl *genl;
>   	struct sockaddr_nl addr;
> @@ -542,7 +545,7 @@ LIB_EXPORT struct l_genl *l_genl_new_default(void)
>
>   	memset(&addr, 0, sizeof(addr));
>   	addr.nl_family = AF_NETLINK;
> -	addr.nl_pid = 0;
> +	addr.nl_pid = pid;
>
>   	if (bind(fd, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
>   		close(fd);
> @@ -569,6 +572,11 @@ LIB_EXPORT struct l_genl *l_genl_new_default(void)
>   	return genl;
>   }
>
> +LIB_EXPORT struct l_genl *l_genl_new_default(void)
> +{
> +	return l_genl_new_default_for_pid(0);
> +}
> +
>   LIB_EXPORT struct l_genl *l_genl_ref(struct l_genl *genl)
>   {
>   	if (unlikely(!genl))
> @@ -1232,6 +1240,9 @@ static void add_membership(struct l_genl *genl, struct genl_mcast *mcast)
>   {
>   	int group = mcast->id;
>
> +	if (!group)
> +		return;
> +
>   	if (mcast->users > 0)
>   		return;
>
> @@ -1301,6 +1312,22 @@ LIB_EXPORT unsigned int l_genl_family_register(struct l_genl_family *family,
>   	return notify->id;
>   }
>
> +
> +LIB_EXPORT unsigned int
> +l_genl_family_register_ucast_listener(struct l_genl_family *family,
> +					l_genl_msg_func_t ucast_listener,
> +					void *user_data,
> +					l_genl_destroy_func_t destroy)
> +{
> +	const char *unicast_group = "unicast";
> +
> +	if (!l_genl_family_has_group(family, unicast_group))
> +		family_add_mcast(family, unicast_group, 0);

So you're adding a multicast family for unicast.  This just doesn't 
compute ;)

> +
> +	return l_genl_family_register(family, unicast_group, ucast_listener,
> +							user_data, destroy);

Funamentally, l_genl is used to send commands and receive responses. 
What you're trying to do is receive commands and send responses.

Whats your plan on how to handle l_genl_msg objects?

How many receivers to you expect?  You can take the easy way out and 
register a single receiver for unicast messages.  Then that receiver 
would be responsible for de-multiplexing the various types of messages 
being received.

Alternatively, the library can handle the de-multiplexing and 
registration/invocation/deregistration of the handlers.

Think l_dbus_send vs l_dbus_interface...

> +}
> +
>   static bool match_notify_id(const void *a, const void *b)
>   {
>   	const struct genl_notify *notify = a;
> diff --git a/ell/genl.h b/ell/genl.h
> index 8f5fd52..650f383 100644
> --- a/ell/genl.h
> +++ b/ell/genl.h
> @@ -25,6 +25,7 @@
>
>   #include <stdbool.h>
>   #include <stdint.h>
> +#include <unistd.h>
>
>   #ifdef __cplusplus
>   extern "C" {
> @@ -37,6 +38,7 @@ struct l_genl;
>
>   struct l_genl *l_genl_new(int fd);
>   struct l_genl *l_genl_new_default(void);
> +struct l_genl *l_genl_new_default_for_pid(pid_t pid);
>
>   struct l_genl *l_genl_ref(struct l_genl *genl);
>   void l_genl_unref(struct l_genl *genl);
> @@ -82,6 +84,8 @@ bool l_genl_attr_recurse(struct l_genl_attr *attr, struct l_genl_attr *nested);
>
>   struct l_genl_family;
>
> +typedef void (*l_genl_msg_func_t)(struct l_genl_msg *msg, void *user_data);
> +
>   struct l_genl_family *l_genl_family_new(struct l_genl *genl, const char *name);
>
>   struct l_genl_family *l_genl_family_ref(struct l_genl_family *family);
> @@ -94,8 +98,6 @@ bool l_genl_family_set_watches(struct l_genl_family *family,
>   				l_genl_watch_func_t vanished,
>   				void *user_data, l_genl_destroy_func_t destroy);
>
> -typedef void (*l_genl_msg_func_t)(struct l_genl_msg *msg, void *user_data);
> -
>   uint32_t l_genl_family_get_version(struct l_genl_family *family);
>
>   bool l_genl_family_can_send(struct l_genl_family *family, uint8_t cmd);
> @@ -112,6 +114,10 @@ bool l_genl_family_has_group(struct l_genl_family *family, const char *group);
>   unsigned int l_genl_family_register(struct l_genl_family *family,
>   				const char *group, l_genl_msg_func_t callback,
>   				void *user_data, l_genl_destroy_func_t destroy);
> +unsigned int l_genl_family_register_ucast_listener(struct l_genl_family *family,
> +					l_genl_msg_func_t ucast_listener,
> +					void *user_data,
> +					l_genl_destroy_func_t destroy);
>   bool l_genl_family_unregister(struct l_genl_family *family, unsigned int id);
>
>   #ifdef __cplusplus
>

Regards,
-Denis