[OE-core] [oe-core][PATCH v4 1/3] udev-extraconf/mount.sh: add support to systemd
Hongzhi, Song
hongzhi.song at windriver.com
Mon Sep 10 08:33:00 UTC 2018
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 at 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 at 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.
>
More information about the Openembedded-core
mailing list