Re: [systemd-devel] [PATCH V3] path-util.c: fix path_is_mount_point() for symlinks

2015-03-10 Thread Lennart Poettering
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

2015-03-10 Thread Lennart Poettering
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

2015-03-10 Thread Lennart Poettering
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

2015-02-20 Thread harald
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