On Mon, Apr 21, 2014 at 07:59:52PM +0200, Kay Sievers wrote:
> On Sun, Apr 20, 2014 at 8:08 PM, Zbigniew Jędrzejewski-Szmek
> <zbys...@in.waw.pl> wrote:
> > On Sun, Apr 20, 2014 at 03:53:05PM +0200, Kay Sievers wrote:
> >> On Sun, Apr 20, 2014 at 5:36 AM, Zbigniew Jędrzejewski-Szmek
> >> <zbys...@in.waw.pl> wrote:
> >>
> >> > Hi Kay,
> >> > it seems that handles are not crucial, and the simplified version below
> >> > works too. Is there something I'm missing?
> >>
> >> The name_to_handle API works properly with bind mounts, it's the more
> >> reliable API.
> >>
> >> It also was intentional to support any filesystem with the prefix
> >> devtmpfs*, like "devtmpfs2" or whatever it might be named in the
> >> future.
> >>
> >> What actual problem are we trying to solve here with not requiring a
> >> common kernel config option? We want/need it in other places too, and
> >> the current fallback logic to figure out the mount point is really not
> >> that great with bind mounts. We just need the fallback for filesystems
> >> which do not support the API, but most common and tmpfs/devtmpfs do.
> >>
> >> I don't necessarily see the point in supporting the idea of the
> >> kernel's uber-configurability and the massive pain it causes for tools
> >> to make things work.
> > Yeah, I'm just trying to reduce the confusion that people experience
> > on kernels without CONFIG_FHANDLE.
> >
> > What about this:
> >
> > --------8<-------------------------------------------------------------
> > Subject: [PATCH] udev: assume /dev is devtmpfs if name_to_handle_at is not
> > implemented
> >
> > We have a bunch of reports from people who have a custom kernel and
> > are confused why udev is not running. This raises the possibility of a
> > false positive when running on a kernel without CONFIG_FHANDLE but in a
> > container. Nevertheless, this caes should be easier to diagnose than
> > the opposite of running on bare metal with the same kernel.
>
> I really don't see the problem with hard-requiring a commonly
> available kernel feature, especially if it involves returning results
> which might be incorrect.
>
> > + log_warning("name_to_handle_at syscall failed,
> > assuming /dev is volatile.");
>
> Libraries should never log for normal operation, only debugging is ok.
>
> > + return true;
> > + }
> >
> > + return false;
> > + }
> >
> > f = fopen("/proc/self/mountinfo", "re");
> > if (!f)
> > @@ -139,8 +146,7 @@ static bool udev_has_devtmpfs(struct udev *udev) {
> > continue;
> >
> > /* accept any name that starts with the currently expected
> > type */
> > - if (startswith(e + 3, "devtmpfs"))
> > - return true;
> > + return startswith(e + 3, "devtmpfs");
> > }
>
> If we really wanted that kind of fallback, we should falling back to
> parsing mountinfo for "devtmpfs" and ignoring the mount_id in this
> loop. But since we need that syscall not only here, I don't think we
> should do that. We should just explain what we need and simply refuse
> to bootup, just like we should refuse to bootup without cgroups
> available.
>
> Supporting less reliable operation modes for something that just needs
> to be configured in the kernel seems the wrong approach, especially
> when it involves filesystem operations on user data like tmpfiles
> does, we just depend on CONFIG_FHANDLE.
OK, what about this:
-------8<--------------------------------------------------------------
udev: warn when name_to_handle_at is not implemented
We have a bunch of reports from people who have a custom kernel and
are confused why udev is not running. Issue a warning on
error. Barring an error in the code, the only error that is possible
is ENOSYS.
https://bugzilla.redhat.com/show_bug.cgi?id=1072966
diff --git a/src/libudev/libudev-monitor.c b/src/libudev/libudev-monitor.c
index 0a2ab82..b9972c9 100644
--- a/src/libudev/libudev-monitor.c
+++ b/src/libudev/libudev-monitor.c
@@ -115,9 +115,11 @@ static bool udev_has_devtmpfs(struct udev *udev) {
int r;
r = name_to_handle_at(AT_FDCWD, "/dev", &h.handle, &mount_id, 0);
- if (r < 0)
+ if (r < 0) {
+ if (errno != EOPNOTSUPP)
+ udev_err(udev, "name_to_handle_at on /dev: %m\n");
return false;
-
+ }
f = fopen("/proc/self/mountinfo", "re");
if (!f)
-------8<--------------------------------------------------------------
Note that this makes missing name_to_handle_at behave similar to failing
socket(), etc, so seems to be in line with surrounding code.
Zbyszek
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
