[systemd-devel] [PATCH] libudev: replace name_to_handle_at with normal sscanf

[Zbigniew Jędrzejewski-Szmek]

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
> <zbyszek at 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
> >> <zbyszek at 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