[RFC] Proof-of-concept for netlink listener for mdev -i

[Isaac Dunham]

On Mon, Mar 16, 2015 at 05:18:27PM +0100, Natanael Copa wrote:
> Ininitial mdev -i patch for busybox which reads messages from stream
> (so it should work with your fif manager):
> 
> http://sprunge.us/ZjLK
> (also attached)
> 
> A few notes:
> - Its completely untested but it compiles
> - it needs the previously mentioned micro protocol ('\0' as message
>   separator)
> - my current nldev hack implementation (netlink socket activator) needs
>   the helper application to read the messages from netlink socket and
>   forward to mdev -i as the previous implementation did.
> - needs be busyboxified.
> 
> Would be nice if someone with more busybox experience than me could
> help clean it up.

> From 3ce1d7f82d39e0aaf4ea53599c6ae2345f157927 Mon Sep 17 00:00:00 2001
> From: Natanael Copa <ncopa at alpinelinux.org>
> Date: Mon, 16 Mar 2015 17:08:06 +0100
> Subject: [PATCH 2/2] mdev: inital support for -i
> 
> Signed-off-by: Natanael Copa <ncopa at alpinelinux.org>
> ---
>  util-linux/mdev.c | 51 +++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 45 insertions(+), 6 deletions(-)
> 
> diff --git a/util-linux/mdev.c b/util-linux/mdev.c
> index 34d32e5..689fe3a 100644
> --- a/util-linux/mdev.c
> +++ b/util-linux/mdev.c
> @@ -71,10 +71,12 @@
>  //kbuild:lib-$(CONFIG_MDEV) += mdev.o
>  
>  //usage:#define mdev_trivial_usage
> -//usage:       "[-s]"
> +//usage:       "[-s|-i]"
>  //usage:#define mdev_full_usage "\n\n"
>  //usage:       "mdev -s is to be run during boot to scan /sys and populate /dev.\n"
>  //usage:       "\n"
> +//usage:       "mdev -i will read events from stdin\n"
> +//usage:       "\n"
>  //usage:       "Bare mdev is a kernel hotplug helper. To activate it:\n"
>  //usage:       "	echo /sbin/mdev >/proc/sys/kernel/hotplug\n"
>  //usage:	IF_FEATURE_MDEV_CONF(
> @@ -1013,7 +1015,7 @@ static void signal_mdevs(unsigned my_pid)
>  	}
>  }
>  
> -static void handle_event(char *temp)
> +static void handle_event(char *temp, int usage_on_error)
>  {
>  	char *fw;
>  	char *seq;
> @@ -1033,8 +1035,11 @@ static void handle_event(char *temp)
>  	G.subsystem = getenv("SUBSYSTEM");
>  	action = getenv("ACTION");
>  	env_devpath = getenv("DEVPATH");
> -	if (!action || !env_devpath /*|| !G.subsystem*/)
> -		bb_show_usage();
> +	if (!action || !env_devpath /*|| !G.subsystem*/) {
> +		if (usage_on_error)
> +			bb_show_usage();
> +		return;
> +	}
>  	fw = getenv("FIRMWARE");
>  	seq = getenv("SEQNUM");
>  	op = index_in_strings(keywords, action);
> @@ -1088,7 +1093,8 @@ int mdev_main(int argc UNUSED_PARAM, char **argv)
>  
>  	/* We can be called as hotplug helper */
>  	/* Kernel cannot provide suitable stdio fds for us, do it ourself */
> -	bb_sanitize_stdio();
> +	if (argv[1] && strcmp(argv[1], "-i") != 0)
> +		bb_sanitize_stdio();
>  
>  	/* Force the configuration file settings exactly */
>  	umask(0);
> @@ -1130,8 +1136,41 @@ int mdev_main(int argc UNUSED_PARAM, char **argv)
>  		recursive_action("/sys/class",
>  			ACTION_RECURSE | ACTION_FOLLOWLINKS,
>  			fileAction, dirAction, temp, 0);
> +	} else if (argv[1] && strcmp(argv[1], "-i") != 0) {

I think this is backwards - strcmp returns 0 on successful matches.

Besides that, I'm wondering if getopt32 pays off at this point.

> +		RESERVE_CONFIG_BUFFER(msgbuf, 16*1024);
> +		struct pollfd fds;
> +		int r;
> +		fds.fd = 0;
> +		fds.events = POLLIN;
> +		while ((r = poll(&fds, 1, 2000)) > 0) {
> +			int i, len, slen;
> +			clearenv();
> +
> +			if (!(fds.revents & POLLIN))
> +				continue;
> +			len = read(fds.fd, msgbuf, sizeof(msgbuf));

Not quite this simple:
As Laurent explained, you will need a separator the moment you get
two messages before you can finish the one you're working on.

If you use \0 as your separator, you can change "i < len" to 
"(i < len) && (msgbuf[i])".
You will then need to move the tail end of msgbuf to the start,
zero out everything else in msgbuf, and adjust the read() so it
doesn't overwrite the remnants of a partial delivery.


> +			for (i = 0; i < len; i += slen + 1) {
> +				char *key, *value;
> +
> +				key = msgbuf + i;
> +				value = strchr(key, '=');
> +				slen = strlen(msgbuf+i);
> +				if (!slen || value == NULL)
> +					continue;
> +
> +				value[0] = '\0';
> +				value++;
> +
> +				setenv(key, value, 1);

This looks like a leak, which is a result of using setenv()/clearenv():

setenv() malloc()s space to save the concatenation of two strings.
clearenv() cannot be expected to free() this (though unsetenv() does).
You will end up leaking the size of the uevents you have processed,
which could be several megabytes during hotplug.

Also, you're doing double work by calling setenv(): it essentially
recreates the string that you just split.

I would think that calling putenv() instead would work better;
you end up making environ point to an array of strings in the memory you
read to, then blank environ[0], then overwrite the memory where
everything was stored.

You may want to save $PATH if you do this, though I don't know if the
kernel-provided default is functionally different from the shell
default.
> +			}
> +			handle_event(temp, 0);
> +			if (fds.revents & POLLHUP)
> +				break;
> +		}
> +		if (r == -1)
> +			return EXIT_FAILURE;
>  	} else {
> -		handle_event(temp);
> +		handle_event(temp, 1);
>  	}
>  
>  	if (ENABLE_FEATURE_CLEAN_UP)
> -- 
> 2.3.2

Thanks,
Isaac Dunham