[PATCH iproute2-next v2] lib/fs: fix issue when {name,open}_to_handle_at() is not implemented

[PATCH iproute2-next v2] lib/fs: fix issue when {name,open}_to_handle_at() is not implemented

[Heiko Thiery]

With commit d5e6ee0dac64b64e the usage of functions name_to_handle_at() and
open_by_handle_at() are introduced. But these function are not available
e.g. in uclibc-ng < 1.0.35. To have a backward compatibility check for the
availability in the configure script and in case of absence do a direct
syscall.

Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
---
v2:
 - small correction to subject
 - removed IP_CONFIG_HANDLE_AT:=y option since it is not required
 - fix indentation in check function
 - removed empty lines (thanks to Petr Vorel)
 - add #define _GNU_SOURCE in check (thanks to Petr Vorel)
 - check only for name_to_handle_at (thanks to Peter Vorel)

 configure | 28 ++++++++++++++++++++++++++++
 lib/fs.c  | 21 +++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/configure b/configure
index 2c363d3b..179eae08 100755
--- a/configure
+++ b/configure
@@ -202,6 +202,31 @@ EOF
     rm -f $TMPDIR/setnstest.c $TMPDIR/setnstest
 }
 
+check_name_to_handle_at()
+{
+    cat >$TMPDIR/name_to_handle_at_test.c <<EOF
+#define _GNU_SOURCE
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+int main(int argc, char **argv)
+{
+	struct file_handle *fhp;
+	int mount_id, flags, dirfd;
+	char *pathname;
+	name_to_handle_at(dirfd, pathname, fhp, &mount_id, flags);
+	return 0;
+}
+EOF
+    if $CC -I$INCLUDE -o $TMPDIR/name_to_handle_at_test $TMPDIR/name_to_handle_at_test.c >/dev/null 2>&1; then
+        echo "yes"
+        echo "CFLAGS += -DHAVE_HANDLE_AT" >>$CONFIG
+    else
+        echo "no"
+    fi
+    rm -f $TMPDIR/name_to_handle_at_test.c $TMPDIR/name_to_handle_at_test
+}
+
 check_ipset()
 {
     cat >$TMPDIR/ipsettest.c <<EOF
@@ -492,6 +517,9 @@ fi
 echo -n "libc has setns: "
 check_setns
 
+echo -n "libc has name_to_handle_at: "
+check_name_to_handle_at
+
 echo -n "SELinux support: "
 check_selinux
 
diff --git a/lib/fs.c b/lib/fs.c
index ee0b130b..feb12864 100644
--- a/lib/fs.c
+++ b/lib/fs.c
@@ -30,6 +30,27 @@
 /* if not already mounted cgroup2 is mounted here for iproute2's use */
 #define MNT_CGRP2_PATH  "/var/run/cgroup2"
 
+
+#ifndef defined HAVE_HANDLE_AT
+struct file_handle {
+	unsigned handle_bytes;
+	int handle_type;
+	unsigned char f_handle[];
+};
+
+int name_to_handle_at(int dirfd, const char *pathname,
+	struct file_handle *handle, int *mount_id, int flags)
+{
+	return syscall(name_to_handle_at, 5, dirfd, pathname, handle,
+	               mount_id, flags);
+}
+
+int open_by_handle_at(int mount_fd, struct file_handle *handle, int flags)
+{
+	return syscall(open_by_handle_at, 3, mount_fd, handle, flags);
+}
+#endif
+
 /* return mount path of first occurrence of given fstype */
 static char *find_fs_mount(const char *fs_to_find)
 {
-- 
2.30.0

Re: [PATCH iproute2-next v2] lib/fs: fix issue when {name,open}_to_handle_at() is not implemented

[Petr Vorel]

Hi Heiko,

...
> +++ b/lib/fs.c
> +int name_to_handle_at(int dirfd, const char *pathname,
> +	struct file_handle *handle, int *mount_id, int flags)
> +{
> +	return syscall(name_to_handle_at, 5, dirfd, pathname, handle,
I overlooked this in v1. name_to_handle_at must be replaced by __NR_name_to_handle_at:
(name_to_handle_at is the function name, not a syscall number).
It also requires to include <sys/syscall.h>:

#include <sys/syscall.h>
...
	return syscall(__NR_name_to_handle_at, 5, dirfd, pathname, handle,
	               mount_id, flags);

> +	               mount_id, flags);
> +}
> +
> +int open_by_handle_at(int mount_fd, struct file_handle *handle, int flags)
> +{
> +	return syscall(open_by_handle_at, 3, mount_fd, handle, flags);
And here needs to be __NR_open_by_handle_at

Kind regards,
Petr

> +}
> +#endif
> +
>  /* return mount path of first occurrence of given fstype */
>  static char *find_fs_mount(const char *fs_to_find)
>  {

Re: [PATCH iproute2-next v2] lib/fs: fix issue when {name,open}_to_handle_at() is not implemented

[Petr Vorel]

Hi,

> +++ b/lib/fs.c
> @@ -30,6 +30,27 @@
>  /* if not already mounted cgroup2 is mounted here for iproute2's use */
>  #define MNT_CGRP2_PATH  "/var/run/cgroup2"

> +
> +#ifndef defined HAVE_HANDLE_AT
This is also wrong, it must be:
#ifndef HAVE_HANDLE_AT

> +struct file_handle {
> +	unsigned handle_bytes;
> +	int handle_type;
> +	unsigned char f_handle[];
> +};
> +
> +int name_to_handle_at(int dirfd, const char *pathname,
> +	struct file_handle *handle, int *mount_id, int flags)
> +{
> +	return syscall(name_to_handle_at, 5, dirfd, pathname, handle,
> +	               mount_id, flags);
Also I overlooked bogus 5 parameter, why is here? Correct is:

	return syscall(__NR_name_to_handle_at, dfd, pathname, handle,
			   mount_id, flags);
> +}
> +
> +int open_by_handle_at(int mount_fd, struct file_handle *handle, int flags)
> +{
> +	return syscall(open_by_handle_at, 3, mount_fd, handle, flags);
And here 3, correct version is is:
	return syscall(__NR_open_by_handle_at, mount_fd, handle, flags);


+ adding at the top:

#ifndef HAVE_HANDLE_AT
# include <sys/syscall.h>
#endif

Kind regards,
Petr

Re: [PATCH iproute2-next v2] lib/fs: fix issue when {name,open}_to_handle_at() is not implemented

[Petr Vorel]

Hi,

> > +++ b/lib/fs.c
> > @@ -30,6 +30,27 @@
> >  /* if not already mounted cgroup2 is mounted here for iproute2's use */
> >  #define MNT_CGRP2_PATH  "/var/run/cgroup2"

> > +
> > +#ifndef defined HAVE_HANDLE_AT
> This is also wrong, it must be:
> #ifndef HAVE_HANDLE_AT

> > +struct file_handle {
> > +	unsigned handle_bytes;
> > +	int handle_type;
> > +	unsigned char f_handle[];
> > +};
> > +
> > +int name_to_handle_at(int dirfd, const char *pathname,
> > +	struct file_handle *handle, int *mount_id, int flags)
> > +{
> > +	return syscall(name_to_handle_at, 5, dirfd, pathname, handle,
> > +	               mount_id, flags);
> Also I overlooked bogus 5 parameter, why is here? Correct is:

> 	return syscall(__NR_name_to_handle_at, dfd, pathname, handle,
> 			   mount_id, flags);
Uh, one more typo on my side, sorry (dfd => dirfd):
	return syscall(__NR_name_to_handle_at, dirfd, pathname, handle,
 			   mount_id, flags);


Kind regards,
Petr

> > +}
> > +
> > +int open_by_handle_at(int mount_fd, struct file_handle *handle, int flags)
> > +{
> > +	return syscall(open_by_handle_at, 3, mount_fd, handle, flags);
> And here 3, correct version is is:
> 	return syscall(__NR_open_by_handle_at, mount_fd, handle, flags);


> + adding at the top:

> #ifndef HAVE_HANDLE_AT
> # include <sys/syscall.h>
> #endif

> Kind regards,
> Petr

Re: [PATCH iproute2-next v2] lib/fs: fix issue when {name,open}_to_handle_at() is not implemented

[Heiko Thiery]

Hi Petr,

Am Fr., 30. Apr. 2021 um 21:29 Uhr schrieb Petr Vorel <petr.vorel@gmail.com>:
>
> Hi,
>
> > > +++ b/lib/fs.c
> > > @@ -30,6 +30,27 @@
> > >  /* if not already mounted cgroup2 is mounted here for iproute2's use */
> > >  #define MNT_CGRP2_PATH  "/var/run/cgroup2"
>
> > > +
> > > +#ifndef defined HAVE_HANDLE_AT
> > This is also wrong, it must be:
> > #ifndef HAVE_HANDLE_AT
>
> > > +struct file_handle {
> > > +   unsigned handle_bytes;
> > > +   int handle_type;
> > > +   unsigned char f_handle[];
> > > +};
> > > +
> > > +int name_to_handle_at(int dirfd, const char *pathname,
> > > +   struct file_handle *handle, int *mount_id, int flags)
> > > +{
> > > +   return syscall(name_to_handle_at, 5, dirfd, pathname, handle,
> > > +                  mount_id, flags);
> > Also I overlooked bogus 5 parameter, why is here? Correct is:
>
> >       return syscall(__NR_name_to_handle_at, dfd, pathname, handle,
> >                          mount_id, flags);
> Uh, one more typo on my side, sorry (dfd => dirfd):
>         return syscall(__NR_name_to_handle_at, dirfd, pathname, handle,
>                            mount_id, flags);
>

Thanks for the review and finding the sloppiness. I really should test
the changes before. Nevertheless, I will prepare a new version and
test it this time.

BR,
-- 
Heiko

Re: [PATCH iproute2-next v2] lib/fs: fix issue when {name,open}_to_handle_at() is not implemented

[Petr Vorel]

> Hi Petr,

> Am Fr., 30. Apr. 2021 um 21:29 Uhr schrieb Petr Vorel <petr.vorel@gmail.com>:

> > Hi,

> > > > +++ b/lib/fs.c
> > > > @@ -30,6 +30,27 @@
> > > >  /* if not already mounted cgroup2 is mounted here for iproute2's use */
> > > >  #define MNT_CGRP2_PATH  "/var/run/cgroup2"

> > > > +
> > > > +#ifndef defined HAVE_HANDLE_AT
> > > This is also wrong, it must be:
> > > #ifndef HAVE_HANDLE_AT

> > > > +struct file_handle {
> > > > +   unsigned handle_bytes;
> > > > +   int handle_type;
> > > > +   unsigned char f_handle[];
> > > > +};
> > > > +
> > > > +int name_to_handle_at(int dirfd, const char *pathname,
> > > > +   struct file_handle *handle, int *mount_id, int flags)
> > > > +{
> > > > +   return syscall(name_to_handle_at, 5, dirfd, pathname, handle,
> > > > +                  mount_id, flags);
> > > Also I overlooked bogus 5 parameter, why is here? Correct is:

> > >       return syscall(__NR_name_to_handle_at, dfd, pathname, handle,
> > >                          mount_id, flags);
> > Uh, one more typo on my side, sorry (dfd => dirfd):
> >         return syscall(__NR_name_to_handle_at, dirfd, pathname, handle,
> >                            mount_id, flags);


> Thanks for the review and finding the sloppiness. I really should test
> the changes before. Nevertheless, I will prepare a new version and
> test it this time.
I tested ss with changed I proposed and it looks like it's ok.
But I run ss on qemu without any daemon running => I'll retest your v3 once you
post it with some daemons running so that the code is really triggered.

Kind regards,
Petr

> BR,

Re: [PATCH iproute2-next v2] lib/fs: fix issue when {name,open}_to_handle_at() is not implemented

[David Ahern]

On 4/30/21 12:26 AM, Heiko Thiery wrote:
> With commit d5e6ee0dac64b64e the usage of functions name_to_handle_at() and

that is a change to ss:

d5e6ee0dac64 ss: introduce cgroup2 cache and helper functions


> open_by_handle_at() are introduced. But these function are not available
> e.g. in uclibc-ng < 1.0.35. To have a backward compatibility check for the
> availability in the configure script and in case of absence do a direct
> syscall.
> 

When you find the proper commit add a Fixes line before the Signed-off-by:

Fixes: <id> ("subject line")
> Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> ---

make sure you cc the author of the original commit.

> v2:
>  - small correction to subject
>  - removed IP_CONFIG_HANDLE_AT:=y option since it is not required
>  - fix indentation in check function
>  - removed empty lines (thanks to Petr Vorel)
>  - add #define _GNU_SOURCE in check (thanks to Petr Vorel)
>  - check only for name_to_handle_at (thanks to Peter Vorel)

you have 3 responses to this commit. Please send an updated patch with
all the changes and the the above comments addressed.

Thanks,

Re: [PATCH iproute2-next v2] lib/fs: fix issue when {name,open}_to_handle_at() is not implemented

[Heiko Thiery]

Hi David,

Am Sa., 1. Mai 2021 um 17:03 Uhr schrieb David Ahern <dsahern@gmail.com>:
>
> On 4/30/21 12:26 AM, Heiko Thiery wrote:
> > With commit d5e6ee0dac64b64e the usage of functions name_to_handle_at() and
>
> that is a change to ss:
>
> d5e6ee0dac64 ss: introduce cgroup2 cache and helper functions
>
>
> > open_by_handle_at() are introduced. But these function are not available
> > e.g. in uclibc-ng < 1.0.35. To have a backward compatibility check for the
> > availability in the configure script and in case of absence do a direct
> > syscall.
> >
>
> When you find the proper commit add a Fixes line before the Signed-off-by:

What do you mean with finding the right commit? This (d5e6ee0dac64) is
the commit that introduced the usage of the missing functions. Or have
I overlooked something?

>
> Fixes: <id> ("subject line")
> > Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> > ---
>
> make sure you cc the author of the original commit.

Ok.

>
> > v2:
> >  - small correction to subject
> >  - removed IP_CONFIG_HANDLE_AT:=y option since it is not required
> >  - fix indentation in check function
> >  - removed empty lines (thanks to Petr Vorel)
> >  - add #define _GNU_SOURCE in check (thanks to Petr Vorel)
> >  - check only for name_to_handle_at (thanks to Peter Vorel)
>
> you have 3 responses to this commit. Please send an updated patch with
> all the changes and the the above comments addressed.

I will implement the comments and send a new version.

> Thanks,

Thanks,
-- 
Heiko

Re: [PATCH iproute2-next v2] lib/fs: fix issue when {name,open}_to_handle_at() is not implemented

[Petr Vorel]

> Hi David,

> Am Sa., 1. Mai 2021 um 17:03 Uhr schrieb David Ahern <dsahern@gmail.com>:

> > On 4/30/21 12:26 AM, Heiko Thiery wrote:
> > > With commit d5e6ee0dac64b64e the usage of functions name_to_handle_at() and

> > that is a change to ss:

> > d5e6ee0dac64 ss: introduce cgroup2 cache and helper functions


> > > open_by_handle_at() are introduced. But these function are not available
> > > e.g. in uclibc-ng < 1.0.35. To have a backward compatibility check for the
> > > availability in the configure script and in case of absence do a direct
> > > syscall.


> > When you find the proper commit add a Fixes line before the Signed-off-by:

> What do you mean with finding the right commit? This (d5e6ee0dac64) is
> the commit that introduced the usage of the missing functions. Or have
> I overlooked something?

Just put into commit message before your Signed-off-by tag this:

Fixes: d5e6ee0d ("ss: introduce cgroup2 cache and helper functions")

> > Fixes: <id> ("subject line")
> > > Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> > > ---


Kind regards,
Petr

Re: [PATCH iproute2-next v2] lib/fs: fix issue when {name,open}_to_handle_at() is not implemented

[David Ahern]

On 5/2/21 5:20 AM, Petr Vorel wrote:
> 
>> What do you mean with finding the right commit? This (d5e6ee0dac64) is
>> the commit that introduced the usage of the missing functions. Or have
>> I overlooked something?

your right; d5e6ee0dac64 is the commit.

> 
> Just put into commit message before your Signed-off-by tag this:
> 
> Fixes: d5e6ee0d ("ss: introduce cgroup2 cache and helper functions")