GNOME Bugzilla – Bug 708378
Advertised new feature: Use systemctl runtime mask to prevent automounting (#701676) doesn't work
Last modified: 2013-12-09 18:12:38 UTC
The commit: Use systemctl runtime mask to prevent automounting (#701676): Should contain 'systemctl list-units' rather than 'systemctl list-unit-files' The latter does not show autogenerated mount files and because of this gparted doesn't mask them. On most distros this means that partitions listed in /etc/fstab will not be protected from automounting which is the behavior that the commit was intended to fix.
Created attachment 256208 [details] [review] Patch to ensure auto-generated /etc/fstab mount points are also prevented from auto-mounting Thank you Jerome for bringing this issue to our attention. It appears that the change to 'systemctl list-units' was lost between the following two patches: Bug 701676 comment #25 and bug 701676 comment #27 Would you be able to test this patch to see if it addresses the auto-mount problem on your system? Instructions on how to build gparted from the git source code can be found at: http://gparted.org/git.php Curtis
Jerome, Are you able to test this patch? Since Ryan Lortie indicated in bug #701676 comment #22 point 2 and you indicate that 'systemctl list-units' should be used, I believe that the patch in comment #1 should address the problem. Curtis
Yes, the patch works for me. Thanks! Another thought: The output of: systemctl list-units --full --all -t mount --no-legend | cut -f1 -d' ' for me is: -.mount Data.mount dev-hugepages.mount dev-mqueue.mount proc-sys-fs-binfmt_misc.mount run-user-1000-gvfs.mount sys-fs-fuse-connections.mount sys-kernel-config.mount sys-kernel-debug.mount tmp.mount With the exception of: '-.mount', 'Data.mount' and 'tmp.mount' the rest of the mount files are virtual filesystems created by systemd and I don't think that we should be masking them. The following command will exclude them MOUNTLIST=`systemctl list-units --full --all -t mount --no-legend | \ grep -v masked | cut -f1 -d' ' | grep -v ^dev-hugepages | \ grep -v ^dev-mqueue | grep -v ^proc-sys-fs-binfmt_misc | \ grep -v ^sys-fs-fuse-connections | grep -v ^sys-kernel-config | \ grep -v ^sys-kernel-debug | grep -v ^run-user-.*-gvfs` If you want a description of these filesystems check the output from systemctl list-units --full --all -t mount
Created attachment 256545 [details] [review] Patch v2 to improve 'systemctl' masking Thank you Jerome for the suggested improvements to the 'systemctl --list-units' mask handling. I have added your suggested changes. Would you be able to re-test this updated patch set?
Patched, compiled and tested: Working as expected, no adverse effects.
Jerome, Thank you for your help with this problem and confirming that the patch works. Mike Fleetwood, When you have a chance, would you be able to review the patch in comment #4? Thanks, Curtis
No problem, thanks for all the work you guys do on gparted. Another quick issue from commit: https://git.gnome.org/browse/gparted/commit/?id=4c109df9b59e55699bd42023cf4007ee359793e9 Found what looks like a copy/paste error: systemctl --runtime unmask --quiet -- $MOUNTLIST The '--runtime' option shouldn't be needed for 'un'masking the mountpoints. It is used for the mask in case there is a crash or some other error, in that case a reboot would fix the issue and the mask wouldn't persist.
Hi Jerome, Thank you for the additional information. I checked the man page for systemctl and it appears that while the --runtime option for unmasking is not required, it should not cause harm. --runtime When used with enable, disable, is-enabled (and related commands), make changes only temporarily, so that they are lost on the next reboot. This will have the effect that changes are not made in subdirectories of /etc but in /run, with identical immediate effects, however, since the latter is lost on reboot, the changes are lost too. Similar, when used with set-property, make changes only temporarily, so that they are lost on the next reboot. To my way of thinking, if all the changes GParted makes with systemctl are lost on reboot then this is a good thing. That way if there are any mistakes (now or with future systemctl versions), a reboot will always set things back to normal. Based on this I think we should leave the patch as-is (e.g., with --runtime on unmask). Curtis
> To my way of thinking, if all the changes GParted makes with systemctl are lost > on reboot then this is a good thing. That way if there are any mistakes (now > or with future systemctl versions), a reboot will always set things back to > normal. I agree, but the only changes made are symlinking (copies of) the mount units to /dev/null systemctl --runtime mask 'is' the defense against changes surviving a reboot. When we undo the mask we want it to be permanent. It makes no sense to temporarily undo this change. I guess my only issue is that the '--runtime' switch with 'unmask' seems to have an undefined behavior now. That could change in the future to output errors or worse. The man page for systemctl seems to only list 4 switches which '--runtime' applies to and unmask isn't among them. Not a big deal, there is ambiguity on the man page, I can see how you'd come to your conclusion, hopefully systemctl docs will be updated to more clearly explain which switches are actually affected by the '--runtime' switch.
Hi Curtis, I have reviewed the patchset and there are a few things which should be changed with number 3: 1) Commit message doesn't, but should, say the command and options used to capture the output. 2) When quoting command output in commit messages don't let git wrap the text at 72 characters. Makes it hard to read. 3) Change that list of 7 piped grep commands into a single egrep command like this: egrep -v '^(dev-hugepages|dev-mqueue|proc-sys-fs-binfmt_misc|...)' Thanks, Mike
Created attachment 256647 [details] [review] Patch v3 to improve 'systemctl' masking Hi Mike and Jerome, Regarding the question of whether or not to use --runtime when unmasking mount units, my concern is that GParted make no permanent changes to any of the systemctl behaviour. Such an event might only occur if a user or utility was making changes to the mount units while GParted was running. The likelihood of this occurring is slim, but I would not want a "permanent" unmask operation from GParted to change something a user performed in another window while GParted was running. That is the main reason why I like --runtime for use in unmasking. Following are responses to suggested improvements to the patch set: 1) Good catch Mike. I should have included the command and options used to create the output in the commit message. 2) My bad. I was trying to make the commit message look better on an 80 char screen. I have changed this back to not wrap the command output text. 3) The code has been changed to use a long egrep command as opposed to multiple piped grep's. In the back of my mind I thought that egrep was one of the commands to be avoided when trying to increase shell portability. Upon checking, egrep is considered safe across shells. See Portable Shell - Utilities in Makefiles [1][2]. The advantage to the multiple grep's is that I could wrap the shell script at 80 characters. The single egrep command spans more than 80 chars and wraps on an 80 character terminal. Mostly this is a personal preference. Since much of the GParted code is longer than 80 characters, I don't think it makes sense to enforce a limit of 80 characters for this one line. Curtis [1] Portable Shell Programming https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html_node/Portable-Shell.html [2] Utilities in Makefiles https://www.gnu.org/prep/standards/html_node/Utilities-in-Makefiles.html#Utilities-in-Makefiles
Hi Curtis, On Fedora 19 I successfully performed this test to confirm the first patch "Change systemctrl to also mask /etc/fstab mount entries" works. 1) Added entry to /etc/fstab: /dev/sdb1 /mnt/1 ext4 default 0 0 2) Reboot (to get systemd to manage the mount) 3) Unmount /mnt/1 4) Run GParted and create new partition /dev/sdb2 6) Display mounted partitions mount | grep /dev/sd Before this patch /dev/sdb1 was being automatically mounted, after this patch /dev/sdb1 remained unmounted. Mechanism: (I'm sure you already know this ...) When libparted modified a partition table it informs the kernel using an ioctl() to remove and readd the partition entry. Systemd is informed of new partition sdb1 (probably via udev) and then because it is a managed mount from /etc/fstab it mounts it. This patch makes the gparted wrapper script inform systemd to not automatically mount any /etc/fstab managed file systems, just while GParted is running. Also made 2 small changes: 1) Added this line to the commit message to answer point (1) doesn't say the command and options used to capture the output from comment #10: $ systemctl list-units --full --all -t mount 2) Reordered the commits to group the two tagged with (#708378) together. Patch set to resolve this bug has been committed to the GParted git repository. Patches can be seen here: Change systemctl to also mask /etc/fstab mount entries (#708378) https://git.gnome.org/browse/gparted/commit/?id=1562994c6de14b205c1262053e1e0517b4a55744 Do not mask virtual file systems when using systemctl (#708378) https://git.gnome.org/browse/gparted/commit/?id=43de8e326a9f6f099e5274619f16039bdc20c1a4 Update install instructions for some GNU/Linux distributions https://git.gnome.org/browse/gparted/commit/?id=d65fb881e25dde3b4d4529463e540bca5105ec4e Thanks, Mike
*** Bug 719665 has been marked as a duplicate of this bug. ***
I made the duplicated https://bugzilla.gnome.org/show_bug.cgi?id=719665 above. I just tried GPARTED_0_16_2-39-g67c0ffb and it seems to work correctly. thanks!
Thank you Kevin for testing and confirming that these patches resolve the problem in your situation. These patches will be included in GParted 0.17.0 which is planned for release on December 9, 2013.
This enhancement was included in the GParted 0.17.0 release on December 9, 2013.