| Message ID | 20210430062632.21304-1-heiko.thiery@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | David Ahern |
| Headers | show |
| Series | [iproute2-next,v2] lib/fs: fix issue when {name,open}_to_handle_at() is not implemented | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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) > {
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
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
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,
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,
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,
> 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,
> 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
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")
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) {
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(+)