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

Treat any mounts below /media as non-essential #91

Closed
wants to merge 1 commit into from

Conversation

mbiebl
Copy link
Contributor

@mbiebl mbiebl commented Jun 8, 2015

Do not consider mounts below /media as essential as the FHS defines this
mount point for removeable media and we shouldn't fail to boot if such
removeable media is not attached.
This can be overridden by using the "fail" mount option.

Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=785525

In Debian, we have quite a few bug reports, where systems failed to boot because /etc/fstab had entries for removeable media which was mounted under /media. Assuming nofail for any mounts below /media would avoid such bad user experiences.
Besides, I couldn't come up with a good reason why we should consider mounts below /media as essential.

@haraldh
Copy link
Member

haraldh commented Jun 8, 2015

IRC comments:

<mjt> why it startswith("/media") and not "/media/" ?  And why it does not apply to, say, /run/media/ as well?
<mjt> and why it can't be overriden in fstab?

@mbiebl
Copy link
Contributor Author

mbiebl commented Jun 8, 2015

I decided to use /media and not /media/, since I've seen fstab entries like
/dev/sdb1 /media ..., i.e. it wasn't always a subdirectory of /media

As for /run/media: I have no objections to extend this to /run/media as well, I just have never seen any fstab entries using /run/media, since those mounts are typically managed via udisks2.

@mbiebl
Copy link
Contributor Author

mbiebl commented Jun 8, 2015

Also, as the commit message says, it can be overridden explicitly via "fail".
I updated the systemd.mount documentation to make that clear.

@haraldh
Copy link
Member

haraldh commented Jun 8, 2015

well, then check for streq("/media") || startswith("/media/")

@mbiebl
Copy link
Contributor Author

mbiebl commented Jun 8, 2015

2015-06-08 13:40 GMT+02:00 Harald Hoyer notifications@github.com:

well, then check for streq("/media") || startswith("/media/")

Is your concern that someone might have a /mediafoo mount point? Seems
unlikely, but I'm happy to update the patch.

Why is it that all of the instruments seeking intelligent life in the
universe are pointed away from Earth?

@haraldh
Copy link
Member

haraldh commented Jun 8, 2015

or

startwith(where, "/media") && (where[6] == 0 || where[6] == '/' ) 

:-)

@mbiebl
Copy link
Contributor Author

mbiebl commented Jun 8, 2015

@haraldh btw: what's the preferred workflow here: add followup commits addressing that or rebasing the branch and repushing? I assume the comments (and the pull request) would get lost if doing the latter?

@haraldh
Copy link
Member

haraldh commented Jun 8, 2015

@mbiebl : squash, rebase the branch and force push. PR is kept and updated

Do not consider mounts below /media as essential as the FHS defines this
mount point for removeable media and we shouldn't fail to boot if such
removeable media is not attached.
This can be overridden by using the "fail" mount option.

Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=785525
@mbiebl
Copy link
Contributor Author

mbiebl commented Jun 8, 2015

thanks for your feedback @haraldh , updated to make sure the directory is either /media/ or a subdirectory of /media.

@poettering
Copy link
Member

Umm, no way. Let's not add something like this. /media might be FHS but certainly not a particularly useful part of it. Removable media is assigned to a seat, and hence a user active on it. As such it should not be mounted to a system-wide namespace, but a per-user namespace, i.e. the way udisks does it.

Mounting removeable media to /media is wrong and unsafe, due to namespace collisions. And we should not add special support for this.

Sorry.

@poettering poettering closed this Jun 8, 2015
@mbiebl
Copy link
Contributor Author

mbiebl commented Jun 8, 2015

@mezcalero This is not about adding special support for /media, but about not breaking boot for existing setups.

@martinpitt
Copy link
Contributor

Mounting removeable media to /media is wrong and unsafe, due to namespace collisions

How so? If people set that up manually, that's their prerogative, and no amount of "it fails under systemd" will stop them from it. udisks creates per-user subdirectories and avoids collisions both that way as well as with dynamically generated suffixes.

@arvidjaar
Copy link
Contributor

If people set that up manually, that's their prerogative

Sure, but then we also should not second-guess their intention and hide legitimate errors. If people set it up manually they can also add "nofail" flag, cannot they?

@mbiebl
Copy link
Contributor Author

mbiebl commented Jun 8, 2015

@arvidjaar the point is that people have existing fstab entries which break the boot with systemd, resulting in a bad user experience and I think we should try harder to avoid that where possible.
Can you come up with a good reason why mounts below /media should be essential?

@poettering
Copy link
Member

/media and /mnt are really something we shouldn't mandate anything about. It's like /foo or /bar, or /waldo. People can mount in there whatever they want, and we should not have any kind of special support for it.

The concept of /media and /mnt is very vague, and it changed over time, for security reasons, and at this point it is not used by any automatism anymore, only by manual configuration. But if it's manual, then people can manually add the "nofail" to the lines if that's the behaviour they want.

If you are concerned about upgrades, than make this something to upgrade from RPM %post or so (or the DEB equivalent), it's relatively easy to sed through /etc/fstab and add an option to it.

I think the whole idea of /media and /mnt is so vaguely defined, that FHS should not mentione it at all but simply allow users to mount things in the root directory wherever they wish as long as they don't call it /run, /usr, /etc and so on...

@karelzak
Copy link
Contributor

karelzak commented Jun 9, 2015

I agree with Lennart. The patch introduces unnecessary complexity, we have never had any special semantic for any specific mountpoints in mount(8) and I don't think systemd has to introduce any hardcoded behaviour according to mountpoint name (well, let's ignore kernel APIs /proc, /sys, etc). It's better to keep such things in config files (=fstab) than in code.

Note that you see the problem because the old "mount -a" (used by sysvinit) is more "promiscuous" and it does not fail if any device does not exist. The systemd concept is different and it requires more precise fstab setting. This is generic systemd behaviour and I don't think we have to make any exceptions (especially according to mountpoint name).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants