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

[Kay Sievers]

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.

Kay