[OpenWrt-Devel,fstools] blockd: don't unmount device when removing it from the list

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

Device gets removed from the list (vlist_delete()) when block calls
"hotplug" method of blockd using ubus. Right after that block unmounts
that device on its own.

blockd shouldn't care about unmounting on its own for following reasons:
1) To avoid code/behavior duplication with block
2) To keep behavior consistent with mounting (blockd doesn't mount)
3) To allow implementing more features in block (e.g. hotplug.d events)

The design should be to:
1) Have block handle (un)mounting
2) Use blockd for providing devices/mounts state (using ubus)
3) Have blockd handle autofs and call block when needed

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 blockd.c | 26 ++------------------------
 1 file changed, 2 insertions(+), 24 deletions(-)

> Op 4 dec. 2018, om 12:32 heeft Rafał Miłecki <zajec5@gmail.com> het volgende geschreven:
> 
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Device gets removed from the list (vlist_delete()) when block calls
> "hotplug" method of blockd using ubus. Right after that block unmounts
> that device on its own.
> 
> blockd shouldn't care about unmounting on its own for following reasons:
> 1) To avoid code/behavior duplication with block
> 2) To keep behavior consistent with mounting (blockd doesn't mount)
> 3) To allow implementing more features in block (e.g. hotplug.d events)
> 
> The design should be to:
> 1) Have block handle (un)mounting
> 2) Use blockd for providing devices/mounts state (using ubus)
> 3) Have blockd handle autofs and call block when needed
Can this cause a transition into a state where a device is (still) mounted but removed from the list, and if so, is that a valid, an admissible state ? Shouldn't block be required to first unmount before calling blockd's hotplug entry ?

> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> blockd.c | 26 ++------------------------
> 1 file changed, 2 insertions(+), 24 deletions(-)
> 
> diff --git a/blockd.c b/blockd.c
> index a5da32c..1379635 100644
> --- a/blockd.c
> +++ b/blockd.c
> @@ -112,34 +112,12 @@ static void
> device_free(struct device *device)
> {
> 	struct blob_attr *data[__MOUNT_MAX];
> -	char *target = NULL;
> -	char *path = NULL, _path[64], *mp;
> 
> 	blobmsg_parse(mount_policy, __MOUNT_MAX, data,
> 		      blob_data(device->msg), blob_len(device->msg));
> 
> -	if (data[MOUNT_AUTOFS]) {
> -		target = device->target;
> -		snprintf(_path, sizeof(_path), "/tmp/run/blockd/%s",
> -			 blobmsg_get_string(data[MOUNT_DEVICE]));
> -		path = _path;
> -	} else {
> -		path = target = device->target;
> -	}
> -
> -	mp = _find_mount_point(device->name);
> -	if (path && mp)
> -		if (umount2(path, MNT_DETACH))
> -			ULOG_ERR("failed to unmount %s\n", path);
> -	free(mp);
> -
> -	if (!target)
> -		return;
> -
> -	if (data[MOUNT_AUTOFS])
> -		unlink(target);
> -	else
> -		rmdir(target);
> +	if (data[MOUNT_AUTOFS] && device->target)
> +		unlink(device->target);
> }
> 
> static void
> -- 
> 2.13.7
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel

On 2018-12-05 13:31, Paul Oranje wrote:
>> Op 4 dec. 2018, om 12:32 heeft Rafał Miłecki <zajec5@gmail.com> het 
>> volgende geschreven:
>> 
>> From: Rafał Miłecki <rafal@milecki.pl>
>> 
>> Device gets removed from the list (vlist_delete()) when block calls
>> "hotplug" method of blockd using ubus. Right after that block unmounts
>> that device on its own.
>> 
>> blockd shouldn't care about unmounting on its own for following 
>> reasons:
>> 1) To avoid code/behavior duplication with blockThe chicken or the 
>> eggThe chicken or the egg
>> 2) To keep behavior consistent with mounting (blockd doesn't mount)
>> 3) To allow implementing more features in block (e.g. hotplug.d 
>> events)
>> 
>> The design should be to:
>> 1) Have block handle (un)mounting
>> 2) Use blockd for providing devices/mounts state (using ubus)
>> 3) Have blockd handle autofs and call block when needed
> Can this cause a transition into a state where a device is (still)
> mounted but removed from the list, and if so, is that a valid, an
> admissible state ? Shouldn't block be required to first unmount before
> calling blockd's hotplug entry ?

The chicken or the egg? ;)

We don't have any mutex/semaphore mechanism in place right now. So
unless that gets implemented, we have to chooce what's better.

I believe the correct order would be to:
1) Stop reporting mounted device
2) Notify all users about unmounting (hotplug.d event I work on)
3) Unmount

That's the safest way that will stop applications from trying to access
device due to blockd incorrectly reporting it as mounted.

On 05/12/2018 13:39, Rafał Miłecki wrote:
> On 2018-12-05 13:31, Paul Oranje wrote:
>>> Op 4 dec. 2018, om 12:32 heeft Rafał Miłecki <zajec5@gmail.com> het 
>>> volgende geschreven:
>>>
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> Device gets removed from the list (vlist_delete()) when block calls
>>> "hotplug" method of blockd using ubus. Right after that block unmounts
>>> that device on its own.
>>>
>>> blockd shouldn't care about unmounting on its own for following 
>>> reasons:
>>> 1) To avoid code/behavior duplication with blockThe chicken or the 
>>> eggThe chicken or the egg
>>> 2) To keep behavior consistent with mounting (blockd doesn't mount)
>>> 3) To allow implementing more features in block (e.g. hotplug.d events)
>>>
>>> The design should be to:
>>> 1) Have block handle (un)mounting
>>> 2) Use blockd for providing devices/mounts state (using ubus)
>>> 3) Have blockd handle autofs and call block when needed
>> Can this cause a transition into a state where a device is (still)
>> mounted but removed from the list, and if so, is that a valid, an
>> admissible state ? Shouldn't block be required to first unmount before
>> calling blockd's hotplug entry ?
>
> The chicken or the egg? ;)
>
> We don't have any mutex/semaphore mechanism in place right now. So
> unless that gets implemented, we have to chooce what's better.
>
> I believe the correct order would be to:
> 1) Stop reporting mounted device
> 2) Notify all users about unmounting (hotplug.d event I work on)
> 3) Unmount
>
> That's the safest way that will stop applications from trying to access
> device due to blockd incorrectly reporting it as mounted.
>
please update the description before pushing

Acked-by: John Crispin <john@phrozen.org>

On 2018-12-06 08:24, John Crispin wrote:
> On 05/12/2018 13:39, Rafał Miłecki wrote:
>> On 2018-12-05 13:31, Paul Oranje wrote:
>>>> Op 4 dec. 2018, om 12:32 heeft Rafał Miłecki <zajec5@gmail.com> het 
>>>> volgende geschreven:
>>>> 
>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>> 
>>>> Device gets removed from the list (vlist_delete()) when block calls
>>>> "hotplug" method of blockd using ubus. Right after that block 
>>>> unmounts
>>>> that device on its own.
>>>> 
>>>> blockd shouldn't care about unmounting on its own for following 
>>>> reasons:
>>>> 1) To avoid code/behavior duplication with blockThe chicken or the 
>>>> eggThe chicken or the egg
>>>> 2) To keep behavior consistent with mounting (blockd doesn't mount)
>>>> 3) To allow implementing more features in block (e.g. hotplug.d 
>>>> events)
>>>> 
>>>> The design should be to:
>>>> 1) Have block handle (un)mounting
>>>> 2) Use blockd for providing devices/mounts state (using ubus)
>>>> 3) Have blockd handle autofs and call block when needed
>>> Can this cause a transition into a state where a device is (still)
>>> mounted but removed from the list, and if so, is that a valid, an
>>> admissible state ? Shouldn't block be required to first unmount 
>>> before
>>> calling blockd's hotplug entry ?
>> 
>> The chicken or the egg? ;)
>> 
>> We don't have any mutex/semaphore mechanism in place right now. So
>> unless that gets implemented, we have to chooce what's better.
>> 
>> I believe the correct order would be to:
>> 1) Stop reporting mounted device
>> 2) Notify all users about unmounting (hotplug.d event I work on)
>> 3) Unmount
>> 
>> That's the safest way that will stop applications from trying to 
>> access
>> device due to blockd incorrectly reporting it as mounted.
>> 
> please update the description before pushing
> 
> Acked-by: John Crispin <john@phrozen.org>

Pushed with updated commit message
https://git.openwrt.org/?p=project/fstools.git;a=commit;h=f6a96865da9162f45ea1d482c794fe14b26ecfe1