[PATCH v4 0/3] udev-extraconf/mount.sh: add support to systemd

[PATCH v4 0/3] udev-extraconf/mount.sh: add support to systemd

[Hongzhi.Song]

v4:
    Fix the recursively dependency for the systemd-mount
v3:
    perfect syntax

Hongzhi.Song (3):
  udev-extraconf/mount.sh: add support to systemd
  udev-extraconf/mount.sh: Fix the recursively dependency for the
    systemd-mount
  udev-extraconf/mount.sh: Skip the entry in /etc/fstab when using the
    systemd-mount

 meta/recipes-core/udev/udev-extraconf/mount.sh | 84 ++++++++++++++++++++++----
 1 file changed, 71 insertions(+), 13 deletions(-)

-- 
2.11.0

[PATCH v4 1/3] udev-extraconf/mount.sh: add support to systemd

[Hongzhi.Song]

Udev-extraconf works correctly with sysvinit in the aspect of automounting
block devices. But it has a serious problem in case of systemd. Block devices
automounted by udev is unaccessible to host space(out of udevd's private
namespace). For example, we cannot format those block devices.

e.g.
    root@qemux86:~# mkfs.ext4 /dev/sda1
    mke2fs 1.43.8
    /dev/sda1 contains a ext4 file system
    last mounted on Tue Apr
    Proceed anyway? (y,N) y
    /dev/sda1 is apparently in use by the system; will not make a filesystem here!

Other distributions has no such problem, because they use a series of rules to
manager block devices. Different types of block devices match different rules.
But udev-extraconf just use one rule, automount.rules, which results in this
problem.

The 'systemd-mount' command is recommended by the systemd community to solve such
problems.

This patch makes use of 'systemd-mount' to solve the above problem.

[YOCTO #12644]

Signed-off-by: Hongzhi.Song <hongzhi.song@windriver.com>
---
 meta/recipes-core/udev/udev-extraconf/mount.sh | 55 +++++++++++++++++++++++---
 1 file changed, 50 insertions(+), 5 deletions(-)

diff --git a/meta/recipes-core/udev/udev-extraconf/mount.sh b/meta/recipes-core/udev/udev-extraconf/mount.sh
index d760328a09..3a72c455e0 100644
--- a/meta/recipes-core/udev/udev-extraconf/mount.sh
+++ b/meta/recipes-core/udev/udev-extraconf/mount.sh
@@ -4,10 +4,26 @@
 #
 # Attempt to mount any added block devices and umount any removed devices
 
+BASE_INIT="`readlink "/sbin/init"`"
+INIT_SYSTEMD="/lib/systemd/systemd"
+
+if [ "x$BASE_INIT" = "x$INIT_SYSTEMD" ];then
+        MOUNT="/usr/bin/systemd-mount"
+        UMOUNT="/usr/bin/systemd-umount"
+
+        if [ -x $MOUNT ] && [ -x $UMOUNT ];
+        then
+                logger "Using systemd-mount to finish mount"
+        else
+                logger "Linux init is using systemd, so please install systemd-mount to finish mount"
+        fi
+else
+        MOUNT="/bin/mount"
+        UMOUNT="/bin/umount"
+fi
 
-MOUNT="/bin/mount"
 PMOUNT="/usr/bin/pmount"
-UMOUNT="/bin/umount"
+
 for line in `grep -h -v ^# /etc/udev/mount.blacklist /etc/udev/mount.blacklist.d/*`
 do
 	if [ ` expr match "$DEVNAME" "$line" ` -gt 0 ];
@@ -17,6 +33,33 @@ do
 	fi
 done
 
+automount_systemd() {
+	name="`basename "$DEVNAME"`"
+
+        ! test -d "/run/media/$name" && mkdir -p "/run/media/$name"
+        # Silent util-linux's version of mounting auto
+        MOUNT="$MOUNT -o silent"
+        
+        # If filesystem type is vfat, change the ownership group to 'disk', and
+        # grant it with  w/r/x permissions.
+        case $ID_FS_TYPE in
+        vfat|fat)
+                MOUNT="$MOUNT -o umask=007,gid=`awk -F':' '/^disk/{print $3}' /etc/group`"
+                ;;
+        # TODO
+        *)
+                ;;
+        esac
+
+        if ! $MOUNT --no-block -t auto $DEVNAME "/run/media/$name"
+        then
+                rm_dir "/run/media/$name"
+        else
+                logger "mount.sh/automount" "systemd-mount of [/run/media/$name] successful"
+                touch "/tmp/.automount-$name"
+        fi
+}
+
 automount() {	
 	name="`basename "$DEVNAME"`"
 
@@ -61,19 +104,21 @@ rm_dir() {
 # No ID_FS_TYPE for cdrom device, yet it should be mounted
 name="`basename "$DEVNAME"`"
 [ -e /sys/block/$name/device/media ] && media_type=`cat /sys/block/$name/device/media`
-
 if [ "$ACTION" = "add" ] && [ -n "$DEVNAME" ] && [ -n "$ID_FS_TYPE" -o "$media_type" = "cdrom" ]; then
 	if [ -x "$PMOUNT" ]; then
 		$PMOUNT $DEVNAME 2> /dev/null
 	elif [ -x $MOUNT ]; then
     		$MOUNT $DEVNAME 2> /dev/null
 	fi
-	
 	# If the device isn't mounted at this point, it isn't
 	# configured in fstab (note the root filesystem can show up as
 	# /dev/root in /proc/mounts, so check the device number too)
 	if expr $MAJOR "*" 256 + $MINOR != `stat -c %d /`; then
-		grep -q "^$DEVNAME " /proc/mounts || automount
+		if [ "`basename $MOUNT`" = "systemd-mount" ];then
+			grep -q "^$DEVNAME " /proc/mounts || automount_systemd
+		else
+			grep -q "^$DEVNAME " /proc/mounts || automount
+		fi
 	fi
 fi
 
-- 
2.11.0

Re: [PATCH v4 1/3] udev-extraconf/mount.sh: add support to systemd

[Martin Hundebøll]

Hi Hongzhi,

I think duplicating all of 'automount' when the only difference is the 
call to 'mount' is unnecessary.

It should be possible to check for system inside the existing 
'automount' (see diff below).

When reading the script I couldn't help cleaning up the special 
treatment of mount from util-linux (busybox accepts '-o silent' too), 
and remove the 'rm_dir' function, which can be replaced by 'rmdir'.

// Martin

diff --git a/meta/recipes-core/udev/udev-extraconf/mount.sh 
b/meta/recipes-core/udev/udev-extraconf/mount.sh
index d760328a09..6f013c5afb 100644
--- a/meta/recipes-core/udev/udev-extraconf/mount.sh
+++ b/meta/recipes-core/udev/udev-extraconf/mount.sh
@@ -20,11 +20,12 @@ done
  automount() {	
  	name="`basename "$DEVNAME"`"

-	! test -d "/run/media/$name" && mkdir -p "/run/media/$name"
-	# Silent util-linux's version of mounting auto
-	if [ "x`readlink $MOUNT`" = "x/bin/mount.util-linux" ] ;
+	mkdir -p "/run/media/$name"
+
+	# Use systemd-mount to avoid being trapped in udev's mount-namespace
+	if [ "$(readlink /sbin/init)" = "/lib/systemd/systemd" ];
  	then
-		MOUNT="$MOUNT -o silent"
+		MOUNT="/usr/bin/systemd-mount --no-block"
  	fi
  	
  	# If filesystem type is vfat, change the ownership group to 'disk', and
@@ -41,22 +42,12 @@ automount() {
  	if ! $MOUNT -t auto $DEVNAME "/run/media/$name"
  	then
  		#logger "mount.sh/automount" "$MOUNT -t auto $DEVNAME 
\"/run/media/$name\" failed!"
-		rm_dir "/run/media/$name"
+		rmdir "/run/media/$name"
  	else
  		logger "mount.sh/automount" "Auto-mount of [/run/media/$name] 
successful"
  		touch "/tmp/.automount-$name"
  	fi
  }
-	
-rm_dir() {
-	# We do not want to rm -r populated directories
-	if test "`find "$1" | wc -l | tr -d " "`" -lt 2 -a -d "$1"
-	then
-		! test -z "$1" && rm -r "$1"
-	else
-		logger "mount.sh/automount" "Not removing non-empty directory [$1]"
-	fi
-}

  # No ID_FS_TYPE for cdrom device, yet it should be mounted
  name="`basename "$DEVNAME"`"

On 2018-08-02 04:50, Hongzhi.Song wrote:
> Udev-extraconf works correctly with sysvinit in the aspect of automounting
> block devices. But it has a serious problem in case of systemd. Block devices
> automounted by udev is unaccessible to host space(out of udevd's private
> namespace). For example, we cannot format those block devices.
> 
> e.g.
>      root@qemux86:~# mkfs.ext4 /dev/sda1
>      mke2fs 1.43.8
>      /dev/sda1 contains a ext4 file system
>      last mounted on Tue Apr
>      Proceed anyway? (y,N) y
>      /dev/sda1 is apparently in use by the system; will not make a filesystem here!
> 
> Other distributions has no such problem, because they use a series of rules to
> manager block devices. Different types of block devices match different rules.
> But udev-extraconf just use one rule, automount.rules, which results in this
> problem.
> 
> The 'systemd-mount' command is recommended by the systemd community to solve such
> problems.
> 
> This patch makes use of 'systemd-mount' to solve the above problem.
> 
> [YOCTO #12644]
> 
> Signed-off-by: Hongzhi.Song <hongzhi.song@windriver.com>
> ---
>   meta/recipes-core/udev/udev-extraconf/mount.sh | 55 +++++++++++++++++++++++---
>   1 file changed, 50 insertions(+), 5 deletions(-)
> 
> diff --git a/meta/recipes-core/udev/udev-extraconf/mount.sh b/meta/recipes-core/udev/udev-extraconf/mount.sh
> index d760328a09..3a72c455e0 100644
> --- a/meta/recipes-core/udev/udev-extraconf/mount.sh
> +++ b/meta/recipes-core/udev/udev-extraconf/mount.sh
> @@ -4,10 +4,26 @@
>   #
>   # Attempt to mount any added block devices and umount any removed devices
>   
> +BASE_INIT="`readlink "/sbin/init"`"
> +INIT_SYSTEMD="/lib/systemd/systemd"
> +
> +if [ "x$BASE_INIT" = "x$INIT_SYSTEMD" ];then
> +        MOUNT="/usr/bin/systemd-mount"
> +        UMOUNT="/usr/bin/systemd-umount"
> +
> +        if [ -x $MOUNT ] && [ -x $UMOUNT ];
> +        then
> +                logger "Using systemd-mount to finish mount"
> +        else
> +                logger "Linux init is using systemd, so please install systemd-mount to finish mount"
> +        fi
> +else
> +        MOUNT="/bin/mount"
> +        UMOUNT="/bin/umount"
> +fi
>   
> -MOUNT="/bin/mount"
>   PMOUNT="/usr/bin/pmount"
> -UMOUNT="/bin/umount"
> +
>   for line in `grep -h -v ^# /etc/udev/mount.blacklist /etc/udev/mount.blacklist.d/*`
>   do
>   	if [ ` expr match "$DEVNAME" "$line" ` -gt 0 ];
> @@ -17,6 +33,33 @@ do
>   	fi
>   done
>   
> +automount_systemd() {
> +	name="`basename "$DEVNAME"`"
> +
> +        ! test -d "/run/media/$name" && mkdir -p "/run/media/$name"
> +        # Silent util-linux's version of mounting auto
> +        MOUNT="$MOUNT -o silent"
> +
> +        # If filesystem type is vfat, change the ownership group to 'disk', and
> +        # grant it with  w/r/x permissions.
> +        case $ID_FS_TYPE in
> +        vfat|fat)
> +                MOUNT="$MOUNT -o umask=007,gid=`awk -F':' '/^disk/{print $3}' /etc/group`"
> +                ;;
> +        # TODO
> +        *)
> +                ;;
> +        esac
> +
> +        if ! $MOUNT --no-block -t auto $DEVNAME "/run/media/$name"
> +        then
> +                rm_dir "/run/media/$name"
> +        else
> +                logger "mount.sh/automount" "systemd-mount of [/run/media/$name] successful"
> +                touch "/tmp/.automount-$name"
> +        fi
> +}
> +
>   automount() {	
>   	name="`basename "$DEVNAME"`"
>   
> @@ -61,19 +104,21 @@ rm_dir() {
>   # No ID_FS_TYPE for cdrom device, yet it should be mounted
>   name="`basename "$DEVNAME"`"
>   [ -e /sys/block/$name/device/media ] && media_type=`cat /sys/block/$name/device/media`
> -
>   if [ "$ACTION" = "add" ] && [ -n "$DEVNAME" ] && [ -n "$ID_FS_TYPE" -o "$media_type" = "cdrom" ]; then
>   	if [ -x "$PMOUNT" ]; then
>   		$PMOUNT $DEVNAME 2> /dev/null
>   	elif [ -x $MOUNT ]; then
>       		$MOUNT $DEVNAME 2> /dev/null
>   	fi
> -	
>   	# If the device isn't mounted at this point, it isn't
>   	# configured in fstab (note the root filesystem can show up as
>   	# /dev/root in /proc/mounts, so check the device number too)
>   	if expr $MAJOR "*" 256 + $MINOR != `stat -c %d /`; then
> -		grep -q "^$DEVNAME " /proc/mounts || automount
> +		if [ "`basename $MOUNT`" = "systemd-mount" ];then
> +			grep -q "^$DEVNAME " /proc/mounts || automount_systemd
> +		else
> +			grep -q "^$DEVNAME " /proc/mounts || automount
> +		fi
>   	fi
>   fi
>   
> 

Re: [PATCH v4 1/3] udev-extraconf/mount.sh: add support to systemd

[Hongzhi, Song]

On 2018年08月03日 00:15, Martin Hundebøll wrote:
> Hi Hongzhi,
>
> I think duplicating all of 'automount' when the only difference is the 
> call to 'mount' is unnecessary.

Hi,

The patch is compatible with both systemd and sysvinit.

And generally speaking, there will be no systemd-mount command when 
using sysvinit.


// Hongzhi

>
> It should be possible to check for system inside the existing 
> 'automount' (see diff below).
>
> When reading the script I couldn't help cleaning up the special 
> treatment of mount from util-linux (busybox accepts '-o silent' too), 
> and remove the 'rm_dir' function, which can be replaced by 'rmdir'.
>
> // Martin
>
> diff --git a/meta/recipes-core/udev/udev-extraconf/mount.sh 
> b/meta/recipes-core/udev/udev-extraconf/mount.sh
> index d760328a09..6f013c5afb 100644
> --- a/meta/recipes-core/udev/udev-extraconf/mount.sh
> +++ b/meta/recipes-core/udev/udev-extraconf/mount.sh
> @@ -20,11 +20,12 @@ done
>  automount() {
>      name="`basename "$DEVNAME"`"
>
> -    ! test -d "/run/media/$name" && mkdir -p "/run/media/$name"
> -    # Silent util-linux's version of mounting auto
> -    if [ "x`readlink $MOUNT`" = "x/bin/mount.util-linux" ] ;
> +    mkdir -p "/run/media/$name"
> +
> +    # Use systemd-mount to avoid being trapped in udev's mount-namespace
> +    if [ "$(readlink /sbin/init)" = "/lib/systemd/systemd" ];
>      then
> -        MOUNT="$MOUNT -o silent"
> +        MOUNT="/usr/bin/systemd-mount --no-block"
>      fi
>
>      # If filesystem type is vfat, change the ownership group to 
> 'disk', and
> @@ -41,22 +42,12 @@ automount() {
>      if ! $MOUNT -t auto $DEVNAME "/run/media/$name"
>      then
>          #logger "mount.sh/automount" "$MOUNT -t auto $DEVNAME 
> \"/run/media/$name\" failed!"
> -        rm_dir "/run/media/$name"
> +        rmdir "/run/media/$name"
>      else
>          logger "mount.sh/automount" "Auto-mount of [/run/media/$name] 
> successful"
>          touch "/tmp/.automount-$name"
>      fi
>  }
> -
> -rm_dir() {
> -    # We do not want to rm -r populated directories
> -    if test "`find "$1" | wc -l | tr -d " "`" -lt 2 -a -d "$1"
> -    then
> -        ! test -z "$1" && rm -r "$1"
> -    else
> -        logger "mount.sh/automount" "Not removing non-empty directory 
> [$1]"
> -    fi
> -}
>
>  # No ID_FS_TYPE for cdrom device, yet it should be mounted
>  name="`basename "$DEVNAME"`"
>
> On 2018-08-02 04:50, Hongzhi.Song wrote:
>> Udev-extraconf works correctly with sysvinit in the aspect of 
>> automounting
>> block devices. But it has a serious problem in case of systemd. Block 
>> devices
>> automounted by udev is unaccessible to host space(out of udevd's private
>> namespace). For example, we cannot format those block devices.
>>
>> e.g.
>>      root@qemux86:~# mkfs.ext4 /dev/sda1
>>      mke2fs 1.43.8
>>      /dev/sda1 contains a ext4 file system
>>      last mounted on Tue Apr
>>      Proceed anyway? (y,N) y
>>      /dev/sda1 is apparently in use by the system; will not make a 
>> filesystem here!
>>
>> Other distributions has no such problem, because they use a series of 
>> rules to
>> manager block devices. Different types of block devices match 
>> different rules.
>> But udev-extraconf just use one rule, automount.rules, which results 
>> in this
>> problem.
>>
>> The 'systemd-mount' command is recommended by the systemd community 
>> to solve such
>> problems.
>>
>> This patch makes use of 'systemd-mount' to solve the above problem.
>>
>> [YOCTO #12644]
>>
>> Signed-off-by: Hongzhi.Song <hongzhi.song@windriver.com>
>> ---
>>   meta/recipes-core/udev/udev-extraconf/mount.sh | 55 
>> +++++++++++++++++++++++---
>>   1 file changed, 50 insertions(+), 5 deletions(-)
>>
>> diff --git a/meta/recipes-core/udev/udev-extraconf/mount.sh 
>> b/meta/recipes-core/udev/udev-extraconf/mount.sh
>> index d760328a09..3a72c455e0 100644
>> --- a/meta/recipes-core/udev/udev-extraconf/mount.sh
>> +++ b/meta/recipes-core/udev/udev-extraconf/mount.sh
>> @@ -4,10 +4,26 @@
>>   #
>>   # Attempt to mount any added block devices and umount any removed 
>> devices
>>   +BASE_INIT="`readlink "/sbin/init"`"
>> +INIT_SYSTEMD="/lib/systemd/systemd"
>> +
>> +if [ "x$BASE_INIT" = "x$INIT_SYSTEMD" ];then
>> +        MOUNT="/usr/bin/systemd-mount"
>> +        UMOUNT="/usr/bin/systemd-umount"
>> +
>> +        if [ -x $MOUNT ] && [ -x $UMOUNT ];
>> +        then
>> +                logger "Using systemd-mount to finish mount"
>> +        else
>> +                logger "Linux init is using systemd, so please 
>> install systemd-mount to finish mount"
>> +        fi
>> +else
>> +        MOUNT="/bin/mount"
>> +        UMOUNT="/bin/umount"
>> +fi
>>   -MOUNT="/bin/mount"
>>   PMOUNT="/usr/bin/pmount"
>> -UMOUNT="/bin/umount"
>> +
>>   for line in `grep -h -v ^# /etc/udev/mount.blacklist 
>> /etc/udev/mount.blacklist.d/*`
>>   do
>>       if [ ` expr match "$DEVNAME" "$line" ` -gt 0 ];
>> @@ -17,6 +33,33 @@ do
>>       fi
>>   done
>>   +automount_systemd() {
>> +    name="`basename "$DEVNAME"`"
>> +
>> +        ! test -d "/run/media/$name" && mkdir -p "/run/media/$name"
>> +        # Silent util-linux's version of mounting auto
>> +        MOUNT="$MOUNT -o silent"
>> +
>> +        # If filesystem type is vfat, change the ownership group to 
>> 'disk', and
>> +        # grant it with  w/r/x permissions.
>> +        case $ID_FS_TYPE in
>> +        vfat|fat)
>> +                MOUNT="$MOUNT -o umask=007,gid=`awk -F':' 
>> '/^disk/{print $3}' /etc/group`"
>> +                ;;
>> +        # TODO
>> +        *)
>> +                ;;
>> +        esac
>> +
>> +        if ! $MOUNT --no-block -t auto $DEVNAME "/run/media/$name"
>> +        then
>> +                rm_dir "/run/media/$name"
>> +        else
>> +                logger "mount.sh/automount" "systemd-mount of 
>> [/run/media/$name] successful"
>> +                touch "/tmp/.automount-$name"
>> +        fi
>> +}
>> +
>>   automount() {
>>       name="`basename "$DEVNAME"`"
>>   @@ -61,19 +104,21 @@ rm_dir() {
>>   # No ID_FS_TYPE for cdrom device, yet it should be mounted
>>   name="`basename "$DEVNAME"`"
>>   [ -e /sys/block/$name/device/media ] && media_type=`cat 
>> /sys/block/$name/device/media`
>> -
>>   if [ "$ACTION" = "add" ] && [ -n "$DEVNAME" ] && [ -n "$ID_FS_TYPE" 
>> -o "$media_type" = "cdrom" ]; then
>>       if [ -x "$PMOUNT" ]; then
>>           $PMOUNT $DEVNAME 2> /dev/null
>>       elif [ -x $MOUNT ]; then
>>               $MOUNT $DEVNAME 2> /dev/null
>>       fi
>> -
>>       # If the device isn't mounted at this point, it isn't
>>       # configured in fstab (note the root filesystem can show up as
>>       # /dev/root in /proc/mounts, so check the device number too)
>>       if expr $MAJOR "*" 256 + $MINOR != `stat -c %d /`; then
>> -        grep -q "^$DEVNAME " /proc/mounts || automount
>> +        if [ "`basename $MOUNT`" = "systemd-mount" ];then
>> +            grep -q "^$DEVNAME " /proc/mounts || automount_systemd
>> +        else
>> +            grep -q "^$DEVNAME " /proc/mounts || automount
>> +        fi
>>       fi
>>   fi
>>
>

Re: [PATCH v4 1/3] udev-extraconf/mount.sh: add support to systemd

[Randy MacLeod]

First, I have a few changes to make the log more clear.

On 08/01/2018 10:50 PM, Hongzhi.Song wrote:
> Udev-extraconf works correctly with sysvinit in the aspect of automounting
> block devices. But it has a serious problem in case of systemd. Block devices
s/. But it/ but it/


> automounted by udev is unaccessible to host space(out of udevd's private
s/is unaccessible/are not accessible/

s/host space(out of udevd's private namespace)/
  /host space(out of udevd's private namespace)/
> namespace). For example, we cannot format those block devices.
> 
> e.g.
>      root@qemux86:~# mkfs.ext4 /dev/sda1
>      mke2fs 1.43.8
>      /dev/sda1 contains a ext4 file system
>      last mounted on Tue Apr
>      Proceed anyway? (y,N) y
>      /dev/sda1 is apparently in use by the system; will not make a filesystem here!
> 
> Other distributions has no such problem, because they use a series of rules to
> manager block devices. Different types of block devices match different rules.
s/manager/manage/
> But udev-extraconf just use one rule, automount.rules, which results in this
s/But udev-extraconf just one rule/
   Note that udev-extraconf has just one file/

> problem.
> 
> The 'systemd-mount' command is recommended by the systemd community to solve such
> problems.
> 
> This patch makes use of 'systemd-mount' to solve the above problem.

Replace the two sentences above with:

As recommended by members of the systemd community, use the
'systemd-mount' command to resolve this problem since it's
intended purpose is to establish (and destroy) transient mount
or auto-mount points using the service manager job queue thereby
eliminating dependencies loops.


I wouldn't normally be so picky but changes to systemd should
be very clear since it's widely used.


Also, it would be good to explain, near the start of your log,
how you are able to reproduce these this defect.
If this is a regression identify where the problem was introduced.


systemd-mount was introduced in systemd-232:
    450442cf93
       add a new tool for creating transient mount and automount units

$ git tag --contains 450442cf93
v232
v233
v234
...

so it's puzzling that we're only getting around to such a change now.
Hopefully your use case will explain that.


> 
> [YOCTO #12644]
> 
> Signed-off-by: Hongzhi.Song <hongzhi.song@windriver.com>
> ---
>   meta/recipes-core/udev/udev-extraconf/mount.sh | 55 +++++++++++++++++++++++---
>   1 file changed, 50 insertions(+), 5 deletions(-)
> 
> diff --git a/meta/recipes-core/udev/udev-extraconf/mount.sh b/meta/recipes-core/udev/udev-extraconf/mount.sh
> index d760328a09..3a72c455e0 100644
> --- a/meta/recipes-core/udev/udev-extraconf/mount.sh
> +++ b/meta/recipes-core/udev/udev-extraconf/mount.sh
> @@ -4,10 +4,26 @@
>   #
>   # Attempt to mount any added block devices and umount any removed devices >
> +BASE_INIT="`readlink "/sbin/init"`"
> +INIT_SYSTEMD="/lib/systemd/systemd"
> +
> +if [ "x$BASE_INIT" = "x$INIT_SYSTEMD" ];then
> +        MOUNT="/usr/bin/systemd-mount"
> +        UMOUNT="/usr/bin/systemd-umount"
> +
> +        if [ -x $MOUNT ] && [ -x $UMOUNT ];
> +        then
> +                logger "Using systemd-mount to finish mount"
Should we be verbose on success?
Do other distros do this?
If not, you could leave the command as a comment for manual debugging,
if you think it's likely to be useful.

> +        else
> +                logger "Linux init is using systemd, so please install systemd-mount to finish mount"

Since systemd-[u]mount is part of the core systemd package,
    packages-split/systemd/usr/bin/systemd-mount
    packages-split/systemd/usr/bin/systemd-umount
this is only possible if we have filesystem corruption.
Best to say so:
    logger "/sbin/init is systemd but /usr/bin/systemd-mount not found."
    logger "Install systemd-mount to be able to mount all filesystems"


Since this will likely never happen, it's tempting to deal with
both systemd-mount and systemd-umount as a group but if by some
strange user error, systemd-umount was removed but systemd-mount
was still present, then we could enable the user to mount the image.
I'm not sure if that's wise since there could be corruption caused
by later failing to umount. It's probably best to require both but
maybe someone has a different point of view.


> +        fi
> +else
> +        MOUNT="/bin/mount"
> +        UMOUNT="/bin/umount"
> +fi
>   
> -MOUNT="/bin/mount"
>   PMOUNT="/usr/bin/pmount"
> -UMOUNT="/bin/umount"
> +
>   for line in `grep -h -v ^# /etc/udev/mount.blacklist /etc/udev/mount.blacklist.d/*`
>   do
>   	if [ ` expr match "$DEVNAME" "$line" ` -gt 0 ];
> @@ -17,6 +33,33 @@ do
>   	fi
>   done
>   
> +automount_systemd() {
> +	name="`basename "$DEVNAME"`"
> +
> +        ! test -d "/run/media/$name" && mkdir -p "/run/media/$name"
> +        # Silent util-linux's version of mounting auto
> +        MOUNT="$MOUNT -o silent"
> +
> +        # If filesystem type is vfat, change the ownership group to 'disk', and
> +        # grant it with  w/r/x permissions.
> +        case $ID_FS_TYPE in
> +        vfat|fat)
> +                MOUNT="$MOUNT -o umask=007,gid=`awk -F':' '/^disk/{print $3}' /etc/group`"
> +                ;;
> +        # TODO

TODO?
How about
            # All other filesystem types fall though
If you are going to leave the TODO in,
outline what remains to be done in the log.

> +        *)
> +                ;;
> +        esac
> +
> +        if ! $MOUNT --no-block -t auto $DEVNAME "/run/media/$name"
> +        then
> +                rm_dir "/run/media/$name"
> +        else
> +                logger "mount.sh/automount" "systemd-mount of [/run/media/$name] successful"
Do you need a space:
                    logger "mount.sh/automount "
to fix the format when the two strings are printed?

Again, are other distros so verbose?
I guess this log could be useful since it is specific:
     [/run/media/$name]
I don't really like the square brackets because I'm darn picky
but if that's in keeping with other systemd/udev-extraconf code,
you can keep it.

There's a long history of unix being quite terse when
there are no errors.

> +                touch "/tmp/.automount-$name"
> +        fi
> +}
> +
>   automount() {	
>   	name="`basename "$DEVNAME"`"
>   
> @@ -61,19 +104,21 @@ rm_dir() {
>   # No ID_FS_TYPE for cdrom device, yet it should be mounted
>   name="`basename "$DEVNAME"`"
>   [ -e /sys/block/$name/device/media ] && media_type=`cat /sys/block/$name/device/media`
> -
>   if [ "$ACTION" = "add" ] && [ -n "$DEVNAME" ] && [ -n "$ID_FS_TYPE" -o "$media_type" = "cdrom" ]; then
>   	if [ -x "$PMOUNT" ]; then
>   		$PMOUNT $DEVNAME 2> /dev/null
>   	elif [ -x $MOUNT ]; then
>       		$MOUNT $DEVNAME 2> /dev/null
>   	fi
> -	
>   	# If the device isn't mounted at this point, it isn't
>   	# configured in fstab (note the root filesystem can show up as
>   	# /dev/root in /proc/mounts, so check the device number too)
>   	if expr $MAJOR "*" 256 + $MINOR != `stat -c %d /`; then
> -		grep -q "^$DEVNAME " /proc/mounts || automount
> +		if [ "`basename $MOUNT`" = "systemd-mount" ];then
> +			grep -q "^$DEVNAME " /proc/mounts || automount_systemd
> +		else
> +			grep -q "^$DEVNAME " /proc/mounts || automount
> +		fi
>   	fi
>   fi
>   
> 


I hope this review helps.
I suspect it was sitting on the list unprocessed in part because
the motivation for the change was not clear.

I don't have time to look at the other two commits tonight.
I can to that tomorrow if you like or you can work with a co-worker
to revise those changes along the lines of the comments I've
made here and send a v5.

-- 
# Randy MacLeod
# Wind River Linux

Re: [PATCH v4 1/3] udev-extraconf/mount.sh: add support to systemd

[Hongzhi, Song]

On 2018年08月24日 10:00, Randy MacLeod wrote:
>
> First, I have a few changes to make the log more clear.
>
> On 08/01/2018 10:50 PM, Hongzhi.Song wrote:
>> Udev-extraconf works correctly with sysvinit in the aspect of 
>> automounting
>> block devices. But it has a serious problem in case of systemd. Block 
>> devices
> s/. But it/ but it/
>
>
>> automounted by udev is unaccessible to host space(out of udevd's private
> s/is unaccessible/are not accessible/
>
> s/host space(out of udevd's private namespace)/
>  /host space(out of udevd's private namespace)/
>> namespace). For example, we cannot format those block devices.
>>
>> e.g.
>>      root@qemux86:~# mkfs.ext4 /dev/sda1
>>      mke2fs 1.43.8
>>      /dev/sda1 contains a ext4 file system
>>      last mounted on Tue Apr
>>      Proceed anyway? (y,N) y
>>      /dev/sda1 is apparently in use by the system; will not make a 
>> filesystem here!
>>
>> Other distributions has no such problem, because they use a series of 
>> rules to
>> manager block devices. Different types of block devices match 
>> different rules.
> s/manager/manage/
>> But udev-extraconf just use one rule, automount.rules, which results 
>> in this
> s/But udev-extraconf just one rule/
>   Note that udev-extraconf has just one file/
>
>> problem.
>>
>> The 'systemd-mount' command is recommended by the systemd community 
>> to solve such
>> problems.
>>
>> This patch makes use of 'systemd-mount' to solve the above problem.
>
> Replace the two sentences above with:
>
> As recommended by members of the systemd community, use the
> 'systemd-mount' command to resolve this problem since it's
> intended purpose is to establish (and destroy) transient mount
> or auto-mount points using the service manager job queue thereby
> eliminating dependencies loops.
>
>
> I wouldn't normally be so picky but changes to systemd should
> be very clear since it's widely used.
>
>
> Also, it would be good to explain, near the start of your log,
> how you are able to reproduce these this defect.


Hi Randy,

I edited the detail steps of reproduce on [Yocto #12644].
I have attached the link on commit log.

I modified the code with your precious recommend.
And I will send review request within interior before commit patch to 
upstream.

Thanks,
--Hongzhi


> If this is a regression identify where the problem was introduced.
>
>
> systemd-mount was introduced in systemd-232:
>    450442cf93
>       add a new tool for creating transient mount and automount units
>
> $ git tag --contains 450442cf93
> v232
> v233
> v234
> ...
>
> so it's puzzling that we're only getting around to such a change now.
> Hopefully your use case will explain that.
>
>
>>
>> [YOCTO #12644]
>>
>> Signed-off-by: Hongzhi.Song <hongzhi.song@windriver.com>
>> ---
>>   meta/recipes-core/udev/udev-extraconf/mount.sh | 55 
>> +++++++++++++++++++++++---
>>   1 file changed, 50 insertions(+), 5 deletions(-)
>>
>> diff --git a/meta/recipes-core/udev/udev-extraconf/mount.sh 
>> b/meta/recipes-core/udev/udev-extraconf/mount.sh
>> index d760328a09..3a72c455e0 100644
>> --- a/meta/recipes-core/udev/udev-extraconf/mount.sh
>> +++ b/meta/recipes-core/udev/udev-extraconf/mount.sh
>> @@ -4,10 +4,26 @@
>>   #
>>   # Attempt to mount any added block devices and umount any removed 
>> devices >
>> +BASE_INIT="`readlink "/sbin/init"`"
>> +INIT_SYSTEMD="/lib/systemd/systemd"
>> +
>> +if [ "x$BASE_INIT" = "x$INIT_SYSTEMD" ];then
>> +        MOUNT="/usr/bin/systemd-mount"
>> +        UMOUNT="/usr/bin/systemd-umount"
>> +
>> +        if [ -x $MOUNT ] && [ -x $UMOUNT ];
>> +        then
>> +                logger "Using systemd-mount to finish mount"
> Should we be verbose on success?
> Do other distros do this?
> If not, you could leave the command as a comment for manual debugging,
> if you think it's likely to be useful.
>
>> +        else
>> +                logger "Linux init is using systemd, so please 
>> install systemd-mount to finish mount"
>
> Since systemd-[u]mount is part of the core systemd package,
>    packages-split/systemd/usr/bin/systemd-mount
>    packages-split/systemd/usr/bin/systemd-umount
> this is only possible if we have filesystem corruption.
> Best to say so:
>    logger "/sbin/init is systemd but /usr/bin/systemd-mount not found."
>    logger "Install systemd-mount to be able to mount all filesystems"
>
>
> Since this will likely never happen, it's tempting to deal with
> both systemd-mount and systemd-umount as a group but if by some
> strange user error, systemd-umount was removed but systemd-mount
> was still present, then we could enable the user to mount the image.
> I'm not sure if that's wise since there could be corruption caused
> by later failing to umount. It's probably best to require both but
> maybe someone has a different point of view.
>
>
>> +        fi
>> +else
>> +        MOUNT="/bin/mount"
>> +        UMOUNT="/bin/umount"
>> +fi
>>   -MOUNT="/bin/mount"
>>   PMOUNT="/usr/bin/pmount"
>> -UMOUNT="/bin/umount"
>> +
>>   for line in `grep -h -v ^# /etc/udev/mount.blacklist 
>> /etc/udev/mount.blacklist.d/*`
>>   do
>>       if [ ` expr match "$DEVNAME" "$line" ` -gt 0 ];
>> @@ -17,6 +33,33 @@ do
>>       fi
>>   done
>>   +automount_systemd() {
>> +    name="`basename "$DEVNAME"`"
>> +
>> +        ! test -d "/run/media/$name" && mkdir -p "/run/media/$name"
>> +        # Silent util-linux's version of mounting auto
>> +        MOUNT="$MOUNT -o silent"
>> +
>> +        # If filesystem type is vfat, change the ownership group to 
>> 'disk', and
>> +        # grant it with  w/r/x permissions.
>> +        case $ID_FS_TYPE in
>> +        vfat|fat)
>> +                MOUNT="$MOUNT -o umask=007,gid=`awk -F':' 
>> '/^disk/{print $3}' /etc/group`"
>> +                ;;
>> +        # TODO
>
> TODO?
> How about
>            # All other filesystem types fall though
> If you are going to leave the TODO in,
> outline what remains to be done in the log.
>
>> +        *)
>> +                ;;
>> +        esac
>> +
>> +        if ! $MOUNT --no-block -t auto $DEVNAME "/run/media/$name"
>> +        then
>> +                rm_dir "/run/media/$name"
>> +        else
>> +                logger "mount.sh/automount" "systemd-mount of 
>> [/run/media/$name] successful"
> Do you need a space:
>                    logger "mount.sh/automount "
> to fix the format when the two strings are printed?
>
> Again, are other distros so verbose?
> I guess this log could be useful since it is specific:
>     [/run/media/$name]
> I don't really like the square brackets because I'm darn picky
> but if that's in keeping with other systemd/udev-extraconf code,
> you can keep it.
>
> There's a long history of unix being quite terse when
> there are no errors.
>
>> +                touch "/tmp/.automount-$name"
>> +        fi
>> +}
>> +
>>   automount() {
>>       name="`basename "$DEVNAME"`"
>>   @@ -61,19 +104,21 @@ rm_dir() {
>>   # No ID_FS_TYPE for cdrom device, yet it should be mounted
>>   name="`basename "$DEVNAME"`"
>>   [ -e /sys/block/$name/device/media ] && media_type=`cat 
>> /sys/block/$name/device/media`
>> -
>>   if [ "$ACTION" = "add" ] && [ -n "$DEVNAME" ] && [ -n "$ID_FS_TYPE" 
>> -o "$media_type" = "cdrom" ]; then
>>       if [ -x "$PMOUNT" ]; then
>>           $PMOUNT $DEVNAME 2> /dev/null
>>       elif [ -x $MOUNT ]; then
>>               $MOUNT $DEVNAME 2> /dev/null
>>       fi
>> -
>>       # If the device isn't mounted at this point, it isn't
>>       # configured in fstab (note the root filesystem can show up as
>>       # /dev/root in /proc/mounts, so check the device number too)
>>       if expr $MAJOR "*" 256 + $MINOR != `stat -c %d /`; then
>> -        grep -q "^$DEVNAME " /proc/mounts || automount
>> +        if [ "`basename $MOUNT`" = "systemd-mount" ];then
>> +            grep -q "^$DEVNAME " /proc/mounts || automount_systemd
>> +        else
>> +            grep -q "^$DEVNAME " /proc/mounts || automount
>> +        fi
>>       fi
>>   fi
>>
>
>
> I hope this review helps.
> I suspect it was sitting on the list unprocessed in part because
> the motivation for the change was not clear.
>
> I don't have time to look at the other two commits tonight.
> I can to that tomorrow if you like or you can work with a co-worker
> to revise those changes along the lines of the comments I've
> made here and send a v5.
>

[PATCH 2/3] udev-extraconf/mount.sh: Fix the recursively dependency for the systemd-mount

[Hongzhi.Song]

If systemd is used, using the systemd-mount to mount the new added disk
partitions forgets to move the codes which tries to mount the partition
by using the configuration in /etc/fstab to the non-systemd function.

And it will cause the systemd-mount try to mount the partition
synchronously and trigger a recursively dependency like the following:
    dev-sda1.device -> run-media-sda1.mount -> dev-sda1.device

Signed-off-by: Kevin Hao <kexin.hao@windriver.com>
Signed-off-by: Hongzhi.Song <hongzhi.song@windriver.com>
---
 meta/recipes-core/udev/udev-extraconf/mount.sh | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/meta/recipes-core/udev/udev-extraconf/mount.sh b/meta/recipes-core/udev/udev-extraconf/mount.sh
index 3a72c455e0..816d975e5e 100644
--- a/meta/recipes-core/udev/udev-extraconf/mount.sh
+++ b/meta/recipes-core/udev/udev-extraconf/mount.sh
@@ -63,6 +63,16 @@ automount_systemd() {
 automount() {	
 	name="`basename "$DEVNAME"`"
 
+	if [ -x "$PMOUNT" ]; then
+		$PMOUNT $DEVNAME 2> /dev/null
+	elif [ -x $MOUNT ]; then
+		$MOUNT $DEVNAME 2> /dev/null
+	fi
+
+	# If the device isn't mounted at this point, it isn't
+	# configured in fstab
+	grep -q "^$DEVNAME " /proc/mounts && return
+
 	! test -d "/run/media/$name" && mkdir -p "/run/media/$name"
 	# Silent util-linux's version of mounting auto
 	if [ "x`readlink $MOUNT`" = "x/bin/mount.util-linux" ] ;
@@ -105,19 +115,13 @@ rm_dir() {
 name="`basename "$DEVNAME"`"
 [ -e /sys/block/$name/device/media ] && media_type=`cat /sys/block/$name/device/media`
 if [ "$ACTION" = "add" ] && [ -n "$DEVNAME" ] && [ -n "$ID_FS_TYPE" -o "$media_type" = "cdrom" ]; then
-	if [ -x "$PMOUNT" ]; then
-		$PMOUNT $DEVNAME 2> /dev/null
-	elif [ -x $MOUNT ]; then
-    		$MOUNT $DEVNAME 2> /dev/null
-	fi
-	# If the device isn't mounted at this point, it isn't
-	# configured in fstab (note the root filesystem can show up as
-	# /dev/root in /proc/mounts, so check the device number too)
+	# Note the root filesystem can show up as /dev/root in /proc/mounts,
+	# so check the device number too
 	if expr $MAJOR "*" 256 + $MINOR != `stat -c %d /`; then
 		if [ "`basename $MOUNT`" = "systemd-mount" ];then
-			grep -q "^$DEVNAME " /proc/mounts || automount_systemd
+			automount_systemd
 		else
-			grep -q "^$DEVNAME " /proc/mounts || automount
+			automount
 		fi
 	fi
 fi
-- 
2.11.0

[PATCH 3/3] udev-extraconf/mount.sh: Skip the entry in /etc/fstab when using the systemd-mount

[Hongzhi.Song]

When using systemd, the systemd-fstab-generator would parse the
/etc/fstab and create the corresponding unit dynamically. So we don't
need to handle the ADD action for the partitions in /etc/fstab.

Signed-off-by: Kevin Hao <kexin.hao@windriver.com>
Signed-off-by: Hongzhi.Song <hongzhi.song@windriver.com>
---
 meta/recipes-core/udev/udev-extraconf/mount.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/meta/recipes-core/udev/udev-extraconf/mount.sh b/meta/recipes-core/udev/udev-extraconf/mount.sh
index 816d975e5e..9796cd7b90 100644
--- a/meta/recipes-core/udev/udev-extraconf/mount.sh
+++ b/meta/recipes-core/udev/udev-extraconf/mount.sh
@@ -36,6 +36,15 @@ done
 automount_systemd() {
 	name="`basename "$DEVNAME"`"
 
+	# Skip the partition which are already in /etc/fstab
+	grep "^[[:space:]]*$DEVNAME" /etc/fstab && return
+	for n in LABEL PARTLABEL UUID PARTUUID; do
+		tmp="$(lsblk -o $n $DEVNAME | sed -e '1d')"
+		test -z "$tmp" && continue
+		tmp="$n=$tmp"
+		grep "^[[:space:]]*$tmp" /etc/fstab && return
+	done
+
         ! test -d "/run/media/$name" && mkdir -p "/run/media/$name"
         # Silent util-linux's version of mounting auto
         MOUNT="$MOUNT -o silent"
-- 
2.11.0

Re: [PATCH v4 0/3] udev-extraconf/mount.sh: add support to systemd

[Hongzhi, Song]

ping

--Hongzhi


On 2018年08月02日 10:50, Hongzhi.Song wrote:
> v4:
>      Fix the recursively dependency for the systemd-mount
> v3:
>      perfect syntax
>
> Hongzhi.Song (3):
>    udev-extraconf/mount.sh: add support to systemd
>    udev-extraconf/mount.sh: Fix the recursively dependency for the
>      systemd-mount
>    udev-extraconf/mount.sh: Skip the entry in /etc/fstab when using the
>      systemd-mount
>
>   meta/recipes-core/udev/udev-extraconf/mount.sh | 84 ++++++++++++++++++++++----
>   1 file changed, 71 insertions(+), 13 deletions(-)
>

[PATCH 0/3 v5] udev-extraconf/mount.sh: add systemd-mount command for systemd

[Hongzhi.Song]

v5:
    perfect a few codes based on v4
v4:
    fix the recursively dependency for the systemd-mount
v3:
    perfect syntax

Hongzhi.Song (3):
  udev-extraconf/mount.sh: add support to systemd
  udev-extraconf/mount.sh: Fix the recursively dependency for the
    systemd-mount
  udev-extraconf/mount.sh: Skip the entry in /etc/fstab when using the
    systemd-moun

 meta/recipes-core/udev/udev-extraconf/mount.sh | 92 +++++++++++++++++++++-----
 1 file changed, 74 insertions(+), 18 deletions(-)

-- 
2.11.0

[PATCH 2/3] udev-extraconf/mount.sh: Fix the recursively dependency for the systemd-mount

[Hongzhi.Song]

If systemd is used, using the systemd-mount to mount the new added disk
partitions forgets to move the codes which tries to mount the partition
by using the configuration in /etc/fstab to the non-systemd function.

And it will cause the systemd-mount try to mount the partition
synchronously and trigger a recursively dependency like the following:
    dev-sda1.device -> run-media-sda1.mount -> dev-sda1.device

Signed-off-by: Kevin Hao <kexin.hao@windriver.com>
Signed-off-by: Hongzhi.Song <hongzhi.song@windriver.com>
---
 meta/recipes-core/udev/udev-extraconf/mount.sh | 27 +++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/meta/recipes-core/udev/udev-extraconf/mount.sh b/meta/recipes-core/udev/udev-extraconf/mount.sh
index 5a877dd7ed..4cb5db3a9b 100644
--- a/meta/recipes-core/udev/udev-extraconf/mount.sh
+++ b/meta/recipes-core/udev/udev-extraconf/mount.sh
@@ -60,7 +60,17 @@ automount_systemd() {
 automount() {
 	name="`basename "$DEVNAME"`"
 
-	! test -d "/run/media/$name" && mkdir -p "/run/media/$name"
+    if [ -x "$PMOUNT" ]; then
+    $PMOUNT $DEVNAME 2> /dev/null
+    elif [ -x $MOUNT ]; then
+    $MOUNT $DEVNAME 2> /dev/null
+    fi
+
+    # If the device isn't mounted at this point, it isn't
+    # configured in fstab
+    grep -q "^$DEVNAME " /proc/mounts && return
+
+    ! test -d "/run/media/$name" && mkdir -p "/run/media/$name"
 	# Silent util-linux's version of mounting auto
 	if [ "x`readlink $MOUNT`" = "x/bin/mount.util-linux" ] ;
 	then
@@ -102,19 +112,14 @@ rm_dir() {
 name="`basename "$DEVNAME"`"
 [ -e /sys/block/$name/device/media ] && media_type=`cat /sys/block/$name/device/media`
 if [ "$ACTION" = "add" ] && [ -n "$DEVNAME" ] && [ -n "$ID_FS_TYPE" -o "$media_type" = "cdrom" ]; then
-	if [ -x "$PMOUNT" ]; then
-		$PMOUNT $DEVNAME 2> /dev/null
-	elif [ -x $MOUNT ]; then
-    		$MOUNT $DEVNAME 2> /dev/null
-	fi
-	# If the device isn't mounted at this point, it isn't
-	# configured in fstab (note the root filesystem can show up as
-	# /dev/root in /proc/mounts, so check the device number too)
+
+    # Note the root filesystem can show up as /dev/root in /proc/mounts,
+    # so check the device number too
 	if expr $MAJOR "*" 256 + $MINOR != `stat -c %d /`; then
 		if [ "`basename $MOUNT`" = "systemd-mount" ];then
-			grep -q "^$DEVNAME " /proc/mounts || automount_systemd
+			automount_systemd
 		else
-			grep -q "^$DEVNAME " /proc/mounts || automount
+			automount
 		fi
 	fi
 fi
-- 
2.11.0