Re: [systemd-devel] [PATCH V3] path-util.c: fix path_is_mount_point() for symlinks
On Tue, 10.03.15 17:29, Lennart Poettering (lenn...@poettering.net) wrote: > On Tue, 10.03.15 17:26, Lennart Poettering (lenn...@poettering.net) wrote: > > > On Fri, 20.02.15 13:25, har...@redhat.com (har...@redhat.com) wrote: > > > > Sorry for the late review! > > > > > --- a/src/shared/path-util.c > > > +++ b/src/shared/path-util.c > > > @@ -456,9 +456,9 @@ int path_is_mount_point(const char *t, bool > > > allow_symlink) { > > > > > > union file_handle_union h = FILE_HANDLE_INIT; > > > int mount_id = -1, mount_id_parent = -1; > > > -_cleanup_free_ char *parent = NULL; > > > struct stat a, b; > > > int r; > > > +_cleanup_close_ int fd = -1; > > > bool nosupp = false; > > > > > > /* We are not actually interested in the file handles, but > > > @@ -468,7 +468,15 @@ int path_is_mount_point(const char *t, bool > > > allow_symlink) { > > > if (path_equal(t, "/")) > > > return 1; > > > > > > -r = name_to_handle_at(AT_FDCWD, t, &h.handle, &mount_id, > > > allow_symlink ? AT_SYMLINK_FOLLOW : 0); > > > +fd = openat(AT_FDCWD, t, > > > O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC|(allow_symlink ? 0 : O_PATH)); > > > +if (fd < 0) { > > > +if (errno == ENOENT) > > > +return 0; > > > > We eat up this error? Why not pass the -ENOENT up? > > Hmm, as I see know the existing code also eats up ENOENT... Not > entirely sure why though. > > But patch looks good then, please commit! I commited this now. Thanks! 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 V3] path-util.c: fix path_is_mount_point() for symlinks
On Tue, 10.03.15 17:26, Lennart Poettering (lenn...@poettering.net) wrote: > On Fri, 20.02.15 13:25, har...@redhat.com (har...@redhat.com) wrote: > > Sorry for the late review! > > > --- a/src/shared/path-util.c > > +++ b/src/shared/path-util.c > > @@ -456,9 +456,9 @@ int path_is_mount_point(const char *t, bool > > allow_symlink) { > > > > union file_handle_union h = FILE_HANDLE_INIT; > > int mount_id = -1, mount_id_parent = -1; > > -_cleanup_free_ char *parent = NULL; > > struct stat a, b; > > int r; > > +_cleanup_close_ int fd = -1; > > bool nosupp = false; > > > > /* We are not actually interested in the file handles, but > > @@ -468,7 +468,15 @@ int path_is_mount_point(const char *t, bool > > allow_symlink) { > > if (path_equal(t, "/")) > > return 1; > > > > -r = name_to_handle_at(AT_FDCWD, t, &h.handle, &mount_id, > > allow_symlink ? AT_SYMLINK_FOLLOW : 0); > > +fd = openat(AT_FDCWD, t, > > O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC|(allow_symlink ? 0 : O_PATH)); > > +if (fd < 0) { > > +if (errno == ENOENT) > > +return 0; > > We eat up this error? Why not pass the -ENOENT up? Hmm, as I see know the existing code also eats up ENOENT... Not entirely sure why though. But patch looks good then, please commit! 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 V3] path-util.c: fix path_is_mount_point() for symlinks
On Fri, 20.02.15 13:25, har...@redhat.com (har...@redhat.com) wrote: Sorry for the late review! > --- a/src/shared/path-util.c > +++ b/src/shared/path-util.c > @@ -456,9 +456,9 @@ int path_is_mount_point(const char *t, bool > allow_symlink) { > > union file_handle_union h = FILE_HANDLE_INIT; > int mount_id = -1, mount_id_parent = -1; > -_cleanup_free_ char *parent = NULL; > struct stat a, b; > int r; > +_cleanup_close_ int fd = -1; > bool nosupp = false; > > /* We are not actually interested in the file handles, but > @@ -468,7 +468,15 @@ int path_is_mount_point(const char *t, bool > allow_symlink) { > if (path_equal(t, "/")) > return 1; > > -r = name_to_handle_at(AT_FDCWD, t, &h.handle, &mount_id, > allow_symlink ? AT_SYMLINK_FOLLOW : 0); > +fd = openat(AT_FDCWD, t, > O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC|(allow_symlink ? 0 : O_PATH)); > +if (fd < 0) { > +if (errno == ENOENT) > +return 0; We eat up this error? Why not pass the -ENOENT up? Otherwise I like this! Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH V3] path-util.c: fix path_is_mount_point() for symlinks
From: Harald Hoyer path_is_mount_point() compares the mount_id of a directory and the mount_id of the parent directory. When following symlinks, the function to get the parent directory does not take the symlink into account. /bin -> /usr/bin with /usr being a mountpoint: mount_id of /bin with AT_SYMLINK_FOLLOW != mount_id of / --- V2 with openat and all relative V3 with _cleanup_close_ src/shared/path-util.c | 30 +- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/src/shared/path-util.c b/src/shared/path-util.c index b9db7f1..5b7fed5 100644 --- a/src/shared/path-util.c +++ b/src/shared/path-util.c @@ -456,9 +456,9 @@ int path_is_mount_point(const char *t, bool allow_symlink) { union file_handle_union h = FILE_HANDLE_INIT; int mount_id = -1, mount_id_parent = -1; -_cleanup_free_ char *parent = NULL; struct stat a, b; int r; +_cleanup_close_ int fd = -1; bool nosupp = false; /* We are not actually interested in the file handles, but @@ -468,7 +468,15 @@ int path_is_mount_point(const char *t, bool allow_symlink) { if (path_equal(t, "/")) return 1; -r = name_to_handle_at(AT_FDCWD, t, &h.handle, &mount_id, allow_symlink ? AT_SYMLINK_FOLLOW : 0); +fd = openat(AT_FDCWD, t, O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC|(allow_symlink ? 0 : O_PATH)); +if (fd < 0) { +if (errno == ENOENT) +return 0; + +return -errno; +} + +r = name_to_handle_at(fd, "", &h.handle, &mount_id, AT_EMPTY_PATH); if (r < 0) { if (errno == ENOSYS) /* This kernel does not support name_to_handle_at() @@ -485,12 +493,9 @@ int path_is_mount_point(const char *t, bool allow_symlink) { return -errno; } -r = path_get_parent(t, &parent); -if (r < 0) -return r; h.handle.handle_bytes = MAX_HANDLE_SZ; -r = name_to_handle_at(AT_FDCWD, parent, &h.handle, &mount_id_parent, AT_SYMLINK_FOLLOW); +r = name_to_handle_at(fd, "..", &h.handle, &mount_id_parent, 0); if (r < 0) if (errno == EOPNOTSUPP) if (nosupp) @@ -509,10 +514,7 @@ int path_is_mount_point(const char *t, bool allow_symlink) { return mount_id != mount_id_parent; fallback: -if (allow_symlink) -r = stat(t, &a); -else -r = lstat(t, &a); +r = fstatat(fd, "", &a, AT_EMPTY_PATH); if (r < 0) { if (errno == ENOENT) @@ -521,14 +523,8 @@ fallback: return -errno; } -free(parent); -parent = NULL; - -r = path_get_parent(t, &parent); -if (r < 0) -return r; -r = stat(parent, &b); +r = fstatat(fd, "..", &b, 0); if (r < 0) return -errno; -- 2.3.0 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel