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

[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

[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

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

[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

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

[harald]

From: Harald Hoyer har...@redhat.com

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