2021-04-30 06:31:46

by Heiko Thiery

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

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 <[email protected]>
---
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


2021-04-30 18:43:59

by Petr Vorel

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

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)
> {

2021-04-30 19:08:32

by Petr Vorel

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

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

2021-04-30 19:30:22

by Petr Vorel

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

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

2021-05-01 15:07:26

by David Ahern

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

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 <[email protected]>
> ---

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,

2021-05-02 08:42:11

by Heiko Thiery

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

Hi Petr,

Am Fr., 30. Apr. 2021 um 21:29 Uhr schrieb Petr Vorel <[email protected]>:
>
> 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

2021-05-02 08:42:11

by Heiko Thiery

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

Hi David,

Am Sa., 1. Mai 2021 um 17:03 Uhr schrieb David Ahern <[email protected]>:
>
> 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 <[email protected]>
> > ---
>
> 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

2021-05-02 11:17:36

by Petr Vorel

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

> Hi Petr,

> Am Fr., 30. Apr. 2021 um 21:29 Uhr schrieb Petr Vorel <[email protected]>:

> > 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,

2021-05-02 11:21:37

by Petr Vorel

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

> Hi David,

> Am Sa., 1. Mai 2021 um 17:03 Uhr schrieb David Ahern <[email protected]>:

> > 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 <[email protected]>
> > > ---


Kind regards,
Petr

2021-05-02 13:11:20

by David Ahern

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

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")