Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mount check fails for some subvolumes contains a dot (.) #14

Merged
merged 4 commits into from
Jun 21, 2023

Conversation

real-user
Copy link

@real-user real-user commented Jun 20, 2023

If btrfs-subvolume is mounted inside a hidden folder then the check "systemctl is-failed {path/containing/a/.dot/mountpoint}" will fail and return fault code 4 (program or service status is unknown).

The command "systemctl -t mount" will only contain the characters \x2e for some subvolumes, for example .snapshots will be \x2esnapshots.mount

However, here is one example from /etc/fstab that will not output \x2e instead of a dot:
UUID=e3aca71c-68c0-4f38-ace9-24cfec405e6a
/home/user1/.local/share/containers btrfs subvol=containers 0 0

This will instead be printed in "systemctl -t mount" as:
home-user1-.local-share-containers.mount

This patch will only replace the dot if it is the first character e.g .snapshots, but will not replace the dot in
/home/user1/.local/share/containers.

I do not know if there are other scenarios where "systemctl -t mount" will output \x2e instead of a dot,
I have only seen it for .snapshots (folders that starts with a "." ).

This issue has not been seen until a recent update of systemctl. On my previous snapshot "systemctl is-failed {/path/not/found.mount}" returned 1, but after the update of systemctl it will return 4 in case the mount-point does not exist.

If btrfs-subvolume is mounted inside a hidden folder then
the check "systemctl is-failed {path/containing/a/.dot/mountpoint}"
will fail and return fault code 4 (program or service status is unknown).

The command "systemctl -t mount" will only contain the characters
\x2e for some subvolumes, for example .snapshots will be \x2esnapshots.mount

Here is one example from /etc/fstab that will not output \x2e instead of a dot:
UUID=e3aca71c-68c0-4f38-ace9-24cfec405e6a \
  /home/user1/.local/share/containers btrfs subvol=containers 0 0
This will instead be printed in "systemctl -t mount" as:
home-user1-.local-share-containers.mount

This patch will only replace the dot if it is the first character
e.g .snapshots, but will not replace the dot in
/home/user1/.local/share/containers.
@laenion
Copy link
Contributor

laenion commented Jun 20, 2023

Or dear, that query really needs improvement. Instead of all the manual sed parsing it would be way better to just ask systemd directly what the expected mount unit name is with

systemd-escape -p "${path}"

Do you want to update your pull request, or should I implement this myself?

If btrfs-subvolume is mounted inside a hidden folder then
the check "systemctl is-failed {path/containing/a/.dot/mountpoint}"
will fail and return fault code 4 (program or service status is unknown).

The command "systemctl -t mount" will only contain the characters
\x2e for some subvolumes, for example .snapshots will be \x2esnapshots.mount

Here is one example from /etc/fstab that will not output \x2e instead of a dot:
UUID=e3aca71c-68c0-4f38-ace9-24cfec405e6a \
  /home/user1/.local/share/containers btrfs subvol=containers 0 0
This will instead be printed in "systemctl -t mount" as:
home-user1-.local-share-containers.mount

This patch uses systemd-escape -p "${path}" to ask systemd what the
expected mount unit name is.
@real-user
Copy link
Author

real-user commented Jun 20, 2023

Hi,

Thanks for the feedback. A much better suggestion indeed. I made a new pull-request.
It is my first time contributing so let me know if I need to change anything else.
Appreciate it.

for i in ${MOUNTS}; do
systemctl is-failed -q $i.mount
path=$(systemd-escape -p "${i}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! Thinking of this, there is still one case where it may fail with the current code: If /etc/fstab contains octal encoded strings (such as a space: \040), then the encoding will still be different from systemd, which encodes this as \x20.

So if ${i} was replaced with $(echo -e ${i)} to first decode the string I think it should really catch all combinations.

Using echo -e in case /etc/fstab contains escape characters
If a user had commented out a line in /etc/fstab
then the parsing in awk would either be wrong
or the test would incorrectly fail.
Fixed the awk command to also handle commented lines.
@real-user
Copy link
Author

real-user commented Jun 21, 2023

Hi, I made the update you proposed.
I also noticed that the code does not handle commented lines in /etc/fstab so I improved the awk command.
It should now ignore commented lines and avoid systemctl is-failed -q "${path}.mount" to pick up some garbage in path.

Let me know if you think it is better.
I have tested it on my system and it seem to work fine.

@laenion
Copy link
Contributor

laenion commented Jun 21, 2023

Good catch, thanks a lot!

Merging now. Thanks a lot for keeping up and implementing all my ideas, and Congratulations on your excellent first contribution :-)

@laenion laenion merged commit fd581f9 into openSUSE:master Jun 21, 2023
@laenion
Copy link
Contributor

laenion commented Jun 22, 2023

Thinking about this again (and while having a look at it anyway), the code still had some more problems with obscure characters, so I also reworked the awk part and replaced it with findmnt now.

bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request Jun 23, 2023
https://build.opensuse.org/request/show/1094705
by user fos + dimstar_suse
- Update to version 1.9
  * Fix failing subvolume mount checks with certain characters in
    mount point [gh#openSUSE/health-checker#14].
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants