[OpenWrt-Devel,fstools,RFC] block: generate hotplug.d mount even "add" after mounting

From: Rafał Miłecki <rafal@milecki.pl>

This is what was implemented in mountd and what some scripts used to
use. It's a pretty generic solution for managing software that may use
e.g. USB storage.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
It's just a RFC for now. It's mainly missing a "remove" event support.
---
 block.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

On 30/11/2018 16:07, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> This is what was implemented in mountd and what some scripts used to
> use. It's a pretty generic solution for managing software that may use
> e.g. USB storage.
>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

looks good, I agree that this is a missing feature compared to mountd

     John

> ---
> It's just a RFC for now. It's mainly missing a "remove" event support.
> ---
>   block.c | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
>
> diff --git a/block.c b/block.c
> index a356315..20c14fe 100644
> --- a/block.c
> +++ b/block.c
> @@ -880,6 +880,31 @@ static int exec_mount(const char *source, const char *target,
>   	return err;
>   }
>   
> +static void hotplug_call_mount(const char *action, const char *device)
> +{
> +	pid_t pid;
> +
> +	pid = fork();
> +	if (!pid) {
> +		char * const argv[] = { "hotplug-call", "mount", (char *)0 };
> +		char actionenv[14];
> +		char deviceenv[32];
> +		char *envp[] = { actionenv, deviceenv, (char *)0 };
> +
> +		snprintf(actionenv, sizeof(actionenv), "ACTION=%s", action);
> +		snprintf(deviceenv, sizeof(deviceenv), "DEVICE=%s", device);
> +
> +		execve("/sbin/hotplug-call", argv, envp);
> +		exit(-1);
> +	} else if (pid > 0) {
> +		int status;
> +
> +		waitpid(pid, &status, 0);
> +		if (WEXITSTATUS(status))
> +			ULOG_ERR("hotplug-call call failed: %d\n", WEXITSTATUS(status));
> +	}
> +}
> +
>   static int handle_mount(const char *source, const char *target,
>                           const char *fstype, struct mount *m)
>   {
> @@ -1079,6 +1104,8 @@ static int mount_device(struct probe_info *pr, int type)
>   
>   	handle_swapfiles(true);
>   
> +	hotplug_call_mount("add", device);
> +
>   	return 0;
>   }
>

Hi,

I really appreciate working on this, thanks!
I just tried to apply this on top of current master but it fails to compile.
Additionally, a  few comments inline below.

Am Freitag, 30. November 2018, 16:07:40 CET schrieb Rafał Miłecki:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This is what was implemented in mountd and what some scripts used to
> use. It's a pretty generic solution for managing software that may use
> e.g. USB storage.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> It's just a RFC for now. It's mainly missing a "remove" event support.
> ---
>  block.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/block.c b/block.c
> index a356315..20c14fe 100644
> --- a/block.c
> +++ b/block.c
> @@ -880,6 +880,31 @@ static int exec_mount(const char *source, const char
> *target, return err;
>  }
> 
> +static void hotplug_call_mount(const char *action, const char *device)
> +{
> +	pid_t pid;
> +
> +	pid = fork();
> +	if (!pid) {
> +		char * const argv[] = { "hotplug-call", "mount", (char *)0 };
Any special reason not to use NULL here?

> +		char actionenv[14];

"ACTION=remove" would just fit, personally I always align to 4 or 8 byte...

> +		char deviceenv[32];
> +		char *envp[] = { actionenv, deviceenv, (char *)0 };

as above, not NULL?

> +		snprintf(actionenv, sizeof(actionenv), "ACTION=%s", action);
> +		snprintf(deviceenv, sizeof(deviceenv), "DEVICE=%s", device);

maybe we should add the mountpoint, too. so a script could easily look for 
files (e.g. firmware updates) below this entry point without the need to
find out the mountpoint itself.

> +
> +		execve("/sbin/hotplug-call", argv, envp);
> +		exit(-1);
EXIT_FAILURE instead of -1?

> +	} else if (pid > 0) {
> +		int status;
> +
> +		waitpid(pid, &status, 0);
> +		if (WEXITSTATUS(status))
> +			ULOG_ERR("hotplug-call call failed: %d\n", WEXITSTATUS(status));
> +	}
> +}
> +
>  static int handle_mount(const char *source, const char *target,
>                          const char *fstype, struct mount *m)
>  {
> @@ -1079,6 +1104,8 @@ static int mount_device(struct probe_info *pr, int
> type)
> 
>  	handle_swapfiles(true);
> 
> +	hotplug_call_mount("add", device);
"devices" not  available at this point -> compile error?

Maybe we also should check that the mount actually was successful before 
firing the hotplug event?

> +
>  	return 0;
>  }

I'm not so familiar with ubus - still on my todo list to investigate deeper:-)
But I also though about firing an event via ubus?

Thanks,
Michael

On Fri, 30 Nov 2018 at 21:01, Michael Heimpold <mhei@heimpold.de> wrote:
> I really appreciate working on this, thanks!
> I just tried to apply this on top of current master but it fails to compile.
> Additionally, a  few comments inline below.

Thanks! You need to apply this on top of the other 4 patches I sent today, see:
https://patchwork.ozlabs.org/project/openwrt/list/?series=79025

You're welcome to review them as well!

On 30.11.2018 21:01, Michael Heimpold wrote:
> Am Freitag, 30. November 2018, 16:07:40 CET schrieb Rafał Miłecki:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> This is what was implemented in mountd and what some scripts used to
>> use. It's a pretty generic solution for managing software that may use
>> e.g. USB storage.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>> It's just a RFC for now. It's mainly missing a "remove" event support.
>> ---
>>   block.c | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/block.c b/block.c
>> index a356315..20c14fe 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -880,6 +880,31 @@ static int exec_mount(const char *source, const char
>> *target, return err;
>>   }
>>
>> +static void hotplug_call_mount(const char *action, const char *device)
>> +{
>> +	pid_t pid;
>> +
>> +	pid = fork();
>> +	if (!pid) {
>> +		char * const argv[] = { "hotplug-call", "mount", (char *)0 };
> Any special reason not to use NULL here?

Copy & paste. I'll switch to the NULL.


>> +		char actionenv[14];
> 
> "ACTION=remove" would just fit, personally I always align to 4 or 8 byte...
> 
>> +		char deviceenv[32];
>> +		char *envp[] = { actionenv, deviceenv, (char *)0 };
> 
> as above, not NULL?
> 
>> +		snprintf(actionenv, sizeof(actionenv), "ACTION=%s", action);
>> +		snprintf(deviceenv, sizeof(deviceenv), "DEVICE=%s", device);
> 
> maybe we should add the mountpoint, too. so a script could easily look for
> files (e.g. firmware updates) below this entry point without the need to
> find out the mountpoint itself.

I like this idea!


>> +
>> +		execve("/sbin/hotplug-call", argv, envp);
>> +		exit(-1);
> EXIT_FAILURE instead of -1?

Maybe. execve should never really return. I think we use exit(-1) everywhere.


>> +	} else if (pid > 0) {
>> +		int status;
>> +
>> +		waitpid(pid, &status, 0);
>> +		if (WEXITSTATUS(status))
>> +			ULOG_ERR("hotplug-call call failed: %d\n", WEXITSTATUS(status));
>> +	}
>> +}
>> +
>>   static int handle_mount(const char *source, const char *target,
>>                           const char *fstype, struct mount *m)
>>   {
>> @@ -1079,6 +1104,8 @@ static int mount_device(struct probe_info *pr, int
>> type)
>>
>>   	handle_swapfiles(true);
>>
>> +	hotplug_call_mount("add", device);
> "devices" not  available at this point -> compile error?
> 
> Maybe we also should check that the mount actually was successful before
> firing the hotplug event?

Few lines above there is error handling.


>> +
>>   	return 0;
>>   }
> 
> I'm not so familiar with ubus - still on my todo list to investigate deeper:-)
> But I also though about firing an event via ubus?

Not sure about possible use case / gain. Maybe send a patch once basic
hotplug.d events are implemented?


Thanks for a review!