Re: [systemd-devel] [PATCH] libudev: replace name_to_handle_at with normal sscanf
On Thu, Apr 24, 2014 at 7:35 AM, Lennart Poettering lenn...@poettering.net wrote: On Thu, 24.04.14 07:28, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote: Have you checked that EOPNOTSUPP is really the error that is returned by name_to_handle_at() if the kernel has the entire syscall disabled? Note that there are two different cases to distuingish here: a file system not supporting the operation, and the kernel not supporting the syscal... It's warning on everything except EOPNOTSUPP. My reasoning is: EOPNOTSUPP - syscall supported, but wrong fs type, - return false ENOSYS - syscall not supported - warn and return false any other error - memory error or other major screw-up - warn and return false Looks good to me then, Kay? Sure, looks all fine. Kay ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] libudev: replace name_to_handle_at with normal sscanf
On Thu, Apr 24, 2014 at 10:31:54AM +0200, Kay Sievers wrote: On Thu, Apr 24, 2014 at 7:35 AM, Lennart Poettering lenn...@poettering.net wrote: On Thu, 24.04.14 07:28, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote: Have you checked that EOPNOTSUPP is really the error that is returned by name_to_handle_at() if the kernel has the entire syscall disabled? Note that there are two different cases to distuingish here: a file system not supporting the operation, and the kernel not supporting the syscal... It's warning on everything except EOPNOTSUPP. My reasoning is: EOPNOTSUPP - syscall supported, but wrong fs type, - return false ENOSYS - syscall not supported - warn and return false any other error - memory error or other major screw-up - warn and return false Looks good to me then, Kay? Sure, looks all fine. Pushed now. Zbyszek ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] libudev: replace name_to_handle_at with normal sscanf
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
Re: [systemd-devel] [PATCH] libudev: replace name_to_handle_at with normal sscanf
On Thu, Apr 24, 2014 at 06:35:58AM +0200, Lennart Poettering wrote:
On Thu, 24.04.14 02:47, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote:
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.
Have you checked that EOPNOTSUPP is really the error that is returned by
name_to_handle_at() if the kernel has the entire syscall disabled? Note
that there are two different cases to distuingish here: a file system
not supporting the operation, and the kernel not supporting the
syscal...
It's warning on everything except EOPNOTSUPP. My reasoning is:
EOPNOTSUPP - syscall supported, but wrong fs type, - return false
ENOSYS - syscall not supported - warn and return false
any other error - memory error or other major screw-up - warn and return false
Looking at path_is_mount_point() suggests ENOSYS is used? Or is that
function checking for the wrong error code?
Zbyszek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] libudev: replace name_to_handle_at with normal sscanf
On Thu, 24.04.14 07:28, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote: Have you checked that EOPNOTSUPP is really the error that is returned by name_to_handle_at() if the kernel has the entire syscall disabled? Note that there are two different cases to distuingish here: a file system not supporting the operation, and the kernel not supporting the syscal... It's warning on everything except EOPNOTSUPP. My reasoning is: EOPNOTSUPP - syscall supported, but wrong fs type, - return false ENOSYS - syscall not supported - warn and return false any other error - memory error or other major screw-up - warn and return false Looks good to me then, Kay? Though I still wonder about the difference between ENOTSUP and EOPNOTSUPP. path_is_mount_point() expectes the former, your patch the latter? Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] libudev: replace name_to_handle_at with normal sscanf
On Thu, 24.04.14 07:35, Lennart Poettering (lenn...@poettering.net) wrote: On Thu, 24.04.14 07:28, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote: Have you checked that EOPNOTSUPP is really the error that is returned by name_to_handle_at() if the kernel has the entire syscall disabled? Note that there are two different cases to distuingish here: a file system not supporting the operation, and the kernel not supporting the syscal... It's warning on everything except EOPNOTSUPP. My reasoning is: EOPNOTSUPP - syscall supported, but wrong fs type, - return false ENOSYS - syscall not supported - warn and return false any other error - memory error or other major screw-up - warn and return false Looks good to me then, Kay? Though I still wonder about the difference between ENOTSUP and EOPNOTSUPP. path_is_mount_point() expectes the former, your patch the latter? Got it, on Linux ENOTSUP is simply an alias for EOPNOTSUPP... Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] libudev: replace name_to_handle_at with normal sscanf
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.
Kay
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] libudev: replace name_to_handle_at with normal sscanf
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. Looking into the not-to-far future, we will hard-require a specific kernel version to provide us with the new cgroups logic and kdbus functionality to boot up properly; I think we should just document and explain what we need, and fail to boot with a proper error, and not try to work around insufficient kernel configs and compiled-out features we rely on. We should turn the current failure into a real error message though, instead of patching things back to less reliable APIs. Btw, the same applies to the !cgroups config, that code should just be removed and we error out with a proper explanation, instead of booting (or even segfault at the moment) a half-working system and pretend systemd would magically find a way to make it work without cgroups. Kay ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] libudev: replace name_to_handle_at with normal sscanf
El 20/04/14 10:53, Kay Sievers escribió: 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 real problem here is that kernel developers took the questionable decision of allowing system calls to be disabled or enabled at compile time. -- Cristian I don't know the key to success, but the key to failure is trying to please everybody. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] libudev: replace name_to_handle_at with normal sscanf
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.
https://bugzilla.redhat.com/show_bug.cgi?id=1072966
Also, if we find a mountpoint with a matching id, and it doesn't have
devtmpfs mounted, return false.
---
src/libudev/libudev-monitor.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/libudev/libudev-monitor.c b/src/libudev/libudev-monitor.c
index 3f7436b..dd5f862 100644
--- a/src/libudev/libudev-monitor.c
+++ b/src/libudev/libudev-monitor.c
@@ -117,9 +117,16 @@ static bool udev_has_devtmpfs(struct udev *udev) {
h = alloca(MAX_HANDLE_SZ);
h-handle_bytes = MAX_HANDLE_SZ;
r = name_to_handle_at(AT_FDCWD, /dev, h, mount_id, 0);
-if (r 0)
-return false;
+if (r 0) {
+if (errno == ENOSYS || errno == ENOTSUP) {
+/* This kernel or file system does not support
+ * name_to_handle_at(). */
+log_warning(name_to_handle_at syscall failed,
assuming /dev is volatile.);
+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);
}
return false;
--
1.8.5.2
8-
?
Zbyszek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] libudev: replace name_to_handle_at with normal sscanf
This has the advantage that we use the same sscanf
pattern as in other places where /proc/self/mountinfo is parsed,
and we avoid bugreports from people who are confused about missing
CONFIG_FHANDLE.
An alternate solution would be to warn when (at runtime) name_to_handle_at
is detected to be missing, but this is used in early boot, and seems
less useful overall.
https://bugzilla.redhat.com/show_bug.cgi?id=1072966
---
Hi Kay,
it seems that handles are not crucial, and the simplified version below
works too. Is there something I'm missing?
Zbyszek
README| 2 +-
src/libudev/libudev-monitor.c | 49 ---
src/shared/path-util.c| 6 +-
3 files changed, 30 insertions(+), 27 deletions(-)
diff --git a/README b/README
index cecbcbf..983f02f 100644
--- a/README
+++ b/README
@@ -51,7 +51,6 @@ REQUIREMENTS:
CONFIG_NET
CONFIG_SYSFS
CONFIG_PROC_FS
- CONFIG_FHANDLE (libudev, mount and bind mount handling)
Udev will fail to work with the legacy layout:
CONFIG_SYSFS_DEPRECATED=n
@@ -79,6 +78,7 @@ REQUIREMENTS:
CONFIG_TMPFS_POSIX_ACL
CONFIG_TMPFS_XATTR
CONFIG_SECCOMP
+ CONFIG_FHANDLE (libudev, mount and bind mount handling)
For systemd-bootchart, several proc debug interfaces are required:
CONFIG_SCHEDSTATS
diff --git a/src/libudev/libudev-monitor.c b/src/libudev/libudev-monitor.c
index 3f7436b..0590002 100644
--- a/src/libudev/libudev-monitor.c
+++ b/src/libudev/libudev-monitor.c
@@ -108,39 +108,46 @@ static struct udev_monitor *udev_monitor_new(struct udev
*udev)
/* we consider udev running when /dev is on devtmpfs */
static bool udev_has_devtmpfs(struct udev *udev) {
-struct file_handle *h;
-int mount_id;
_cleanup_fclose_ FILE *f = NULL;
-char line[LINE_MAX], *e;
-int r;
-
-h = alloca(MAX_HANDLE_SZ);
-h-handle_bytes = MAX_HANDLE_SZ;
-r = name_to_handle_at(AT_FDCWD, /dev, h, mount_id, 0);
-if (r 0)
-return false;
-
+int i;
f = fopen(/proc/self/mountinfo, re);
if (!f)
return false;
-FOREACH_LINE(line, f, return false) {
-int mid;
-
-if (sscanf(line, %i, mid) != 1)
-continue;
+for (i = 1;; i++) {
+_cleanup_free_ char *where = NULL, *fstype = NULL;
+int k;
+
+k = fscanf(f,
+ %*s/* (1) mount id */
+ %*s/* (2) parent id */
+ %*s/* (3) major:minor */
+ %*s/* (4) root */
+ %ms/* (5) mount point */
+ %*s/* (6) mount options */
+ %*[^-] /* (7) optional fields */
+ - /* (8) separator */
+ %ms/* (9) file system type */
+ %*s/* (10) mount source */
+ %*s/* (11) mount options 2 */
+ %*[^\n], /* some rubbish at the end */
+ where,
+ fstype);
+
+if (k == EOF)
+break;
-if (mid != mount_id)
+if (k != 2) {
+log_warning(Failed to parse
/proc/self/mountinfo:%u., i);
continue;
+}
-e = strstr(line, - );
-if (!e)
+if (!streq(where, /dev))
continue;
/* accept any name that starts with the currently expected
type */
-if (startswith(e + 3, devtmpfs))
-return true;
+return streq(fstype, devtmpfs);
}
return false;
diff --git a/src/shared/path-util.c b/src/shared/path-util.c
index e35d7f8..5beb5f8 100644
--- a/src/shared/path-util.c
+++ b/src/shared/path-util.c
@@ -324,7 +324,7 @@ bool path_equal(const char *a, const char *b) {
}
int path_is_mount_point(const char *t, bool allow_symlink) {
-char *parent;
+_cleanup_free_ char *parent = NULL;
int r;
struct file_handle *h;
int mount_id, mount_id_parent;
@@ -360,8 +360,6 @@ int path_is_mount_point(const char *t, bool allow_symlink) {
h-handle_bytes = MAX_HANDLE_SZ;
r = name_to_handle_at(AT_FDCWD, parent, h, mount_id_parent, 0);
-free(parent);
-
if (r 0) {
/* The parent can't do name_to_handle_at() but the
* directory we are interested in can? If so, it must
@@ -392,8 +390,6 @@ fallback:
return r;
r = lstat(parent, b);
-free(parent);
-
