| Message ID | 20250304075846.66563-1-louis@kragniz.eu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/5] tools/nolibc: add support for openat(2) | expand |
Hi Louis, On Tue, Mar 04, 2025 at 07:58:15AM +0000, Louis Taylor wrote: > openat is useful to avoid needing to construct relative paths, so expose > a wrapper for using it directly. Reviewed the whole series, no comments from me. Let's wait for Thomas to double-check but for me it's OK: Acked-by: Willy Tarreau <w@1wt.eu> Thomas, since you already have plenty of changes in queue, do you mind if I let you pick this series ? Thanks! Willy
On 2025-03-04 07:58:15+0000, Louis Taylor wrote: > openat is useful to avoid needing to construct relative paths, so expose > a wrapper for using it directly. > > Signed-off-by: Louis Taylor <louis@kragniz.eu> Looks good. I have some tiny nitpicks inline, but if you prefer I can also pick it up as-is. Acked-by: Thomas Weißschuh <linux@weissschuh.net> > --- > tools/include/nolibc/sys.h | 25 ++++++++++++++++++++ > tools/testing/selftests/nolibc/nolibc-test.c | 21 ++++++++++++++++ > 2 files changed, 46 insertions(+) > > diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h > index 8f44c33b1213..3cd938f9abda 100644 > --- a/tools/include/nolibc/sys.h > +++ b/tools/include/nolibc/sys.h > @@ -765,6 +765,31 @@ int mount(const char *src, const char *tgt, > return __sysret(sys_mount(src, tgt, fst, flags, data)); > } > > +/* > + * int openat(int dirfd, const char *path, int flags[, mode_t mode]); > + */ > + > +static __attribute__((unused)) > +int sys_openat(int dirfd, const char *path, int flags, mode_t mode) > +{ > + return my_syscall4(__NR_openat, dirfd, path, flags, mode); > +} > + > +static __attribute__((unused)) > +int openat(int dirfd, const char *path, int flags, ...) > +{ > + mode_t mode = 0; > + > + if (flags & O_CREAT) { > + va_list args; > + > + va_start(args, flags); > + mode = va_arg(args, mode_t); > + va_end(args); > + } > + > + return __sysret(sys_openat(dirfd, path, flags, mode)); > +} > > /* > * int open(const char *path, int flags[, mode_t mode]); > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c > index 79c3e6a845f3..2a1629938dd6 100644 > --- a/tools/testing/selftests/nolibc/nolibc-test.c > +++ b/tools/testing/selftests/nolibc/nolibc-test.c > @@ -1028,6 +1028,26 @@ int test_rlimit(void) > return 0; > } > > +static int test_openat(void) As mentioned in my other mail, this in fact should not be static. Sorry for the back and forth. > +{ > + int dev; > + int null; Can be on a single line. > + > + dev = openat(AT_FDCWD, "/dev", O_DIRECTORY); > + if (dev < 0) > + return -1; > + > + null = openat(dev, "null", 0); As per the standard: The argument flags must include one of the following access modes: O_RDONLY, O_WRONLY, or O_RDWR. The other tests don't do it either and on Linux it doesn't matter because O_RDONLY == 0. But if we are here anyways. close(dev) could be moved here for some saved lines. > + if (null < 0) { > + close(dev); > + return -1; > + } > + > + close(dev); > + close(null); > + > + return 0; > +} > > /* Run syscall tests between IDs <min> and <max>. > * Return 0 on success, non-zero on failure. > @@ -1116,6 +1136,7 @@ int run_syscall(int min, int max) > CASE_TEST(mmap_munmap_good); EXPECT_SYSZR(1, test_mmap_munmap()); break; > CASE_TEST(open_tty); EXPECT_SYSNE(1, tmp = open("/dev/null", 0), -1); if (tmp != -1) close(tmp); break; > CASE_TEST(open_blah); EXPECT_SYSER(1, tmp = open("/proc/self/blah", 0), -1, ENOENT); if (tmp != -1) close(tmp); break; > + CASE_TEST(openat_dir); EXPECT_SYSNE(1, test_openat(), -1); break; EXPECT_SYSZR() should work here. > CASE_TEST(pipe); EXPECT_SYSZR(1, test_pipe()); break; > CASE_TEST(poll_null); EXPECT_SYSZR(1, poll(NULL, 0, 0)); break; > CASE_TEST(poll_stdout); EXPECT_SYSNE(1, ({ struct pollfd fds = { 1, POLLOUT, 0}; poll(&fds, 1, 0); }), -1); break; > -- > 2.45.2 >
Hi Willy, On 2025-03-04 09:11:16+0100, Willy Tarreau wrote: > On Tue, Mar 04, 2025 at 07:58:15AM +0000, Louis Taylor wrote: > > openat is useful to avoid needing to construct relative paths, so expose > > a wrapper for using it directly. > > Reviewed the whole series, no comments from me. Let's wait for Thomas > to double-check but for me it's OK: > > Acked-by: Willy Tarreau <w@1wt.eu> Fairly happy with it, only some tiny nitpicks. > Thomas, since you already have plenty of changes in queue, do you mind > if I let you pick this series ? Sure. Also we need to send a PR to Shuah soon. I have some patches I want to get into it. In case you missed it: https://lore.kernel.org/lkml/20250305-nolibc-asm-headers-v1-1-f2053def2ee7@linutronix.de/ Also the removal of the constructor order validation. Will you send a patch or should I? Thomas
Hi Thomas, On Thu, Mar 06, 2025 at 06:22:45PM +0100, Thomas Weißschuh wrote: > Hi Willy, > > On 2025-03-04 09:11:16+0100, Willy Tarreau wrote: > > On Tue, Mar 04, 2025 at 07:58:15AM +0000, Louis Taylor wrote: > > > openat is useful to avoid needing to construct relative paths, so expose > > > a wrapper for using it directly. > > > > Reviewed the whole series, no comments from me. Let's wait for Thomas > > to double-check but for me it's OK: > > > > Acked-by: Willy Tarreau <w@1wt.eu> > > Fairly happy with it, only some tiny nitpicks. Thanks for double-checking, you caught way more things than me! > > Thomas, since you already have plenty of changes in queue, do you mind > > if I let you pick this series ? > > Sure. > > Also we need to send a PR to Shuah soon. > I have some patches I want to get into it. > In case you missed it: > https://lore.kernel.org/lkml/20250305-nolibc-asm-headers-v1-1-f2053def2ee7@linutronix.de/ Yes I noticed them, these days I'm totally overwhelmed and tend to only be able to have a look at them during my week-ends :-( > Also the removal of the constructor order validation. Yes, not sure I'll have time for it yet :-( I can try, though! > Will you send a patch or should I? As last time it will greatly help me if you take care of it. I feel like I'm leaving you with all the work these days :-/ Cheers, Willy
On Thu Mar 6, 2025 at 5:10 PM GMT, Thomas Weißschuh wrote: > On 2025-03-04 07:58:15+0000, Louis Taylor wrote: > > openat is useful to avoid needing to construct relative paths, so expose > > a wrapper for using it directly. > > > > Signed-off-by: Louis Taylor <louis@kragniz.eu> > > Looks good. I have some tiny nitpicks inline, > but if you prefer I can also pick it up as-is. > > Acked-by: Thomas Weißschuh <linux@weissschuh.net> No problem, I'll send another version. > > --- > > tools/include/nolibc/sys.h | 25 ++++++++++++++++++++ > > tools/testing/selftests/nolibc/nolibc-test.c | 21 ++++++++++++++++ > > 2 files changed, 46 insertions(+) > > > > diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h > > index 8f44c33b1213..3cd938f9abda 100644 > > --- a/tools/include/nolibc/sys.h > > +++ b/tools/include/nolibc/sys.h > > @@ -765,6 +765,31 @@ int mount(const char *src, const char *tgt, > > return __sysret(sys_mount(src, tgt, fst, flags, data)); > > } > > > > +/* > > + * int openat(int dirfd, const char *path, int flags[, mode_t mode]); > > + */ > > + > > +static __attribute__((unused)) > > +int sys_openat(int dirfd, const char *path, int flags, mode_t mode) > > +{ > > + return my_syscall4(__NR_openat, dirfd, path, flags, mode); > > +} > > + > > +static __attribute__((unused)) > > +int openat(int dirfd, const char *path, int flags, ...) > > +{ > > + mode_t mode = 0; > > + > > + if (flags & O_CREAT) { > > + va_list args; > > + > > + va_start(args, flags); > > + mode = va_arg(args, mode_t); > > + va_end(args); > > + } > > + > > + return __sysret(sys_openat(dirfd, path, flags, mode)); > > +} > > > > /* > > * int open(const char *path, int flags[, mode_t mode]); > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c > > index 79c3e6a845f3..2a1629938dd6 100644 > > --- a/tools/testing/selftests/nolibc/nolibc-test.c > > +++ b/tools/testing/selftests/nolibc/nolibc-test.c > > @@ -1028,6 +1028,26 @@ int test_rlimit(void) > > return 0; > > } > > > > +static int test_openat(void) > > As mentioned in my other mail, this in fact should not be static. > Sorry for the back and forth. Ah that's fine, I'll drop the other patch as well. > > +{ > > + int dev; > > + int null; > > Can be on a single line. > > > + > > + dev = openat(AT_FDCWD, "/dev", O_DIRECTORY); > > + if (dev < 0) > > + return -1; > > + > > + null = openat(dev, "null", 0); > > As per the standard: > > The argument flags must include one of the following access modes: > O_RDONLY, O_WRONLY, or O_RDWR. > > The other tests don't do it either and on Linux it doesn't matter > because O_RDONLY == 0. But if we are here anyways. > > close(dev) could be moved here for some saved lines. > > > + if (null < 0) { > > + close(dev); > > + return -1; > > + } > > + > > + close(dev); > > + close(null); > > + > > + return 0; > > +} > > > > /* Run syscall tests between IDs <min> and <max>. > > * Return 0 on success, non-zero on failure. > > @@ -1116,6 +1136,7 @@ int run_syscall(int min, int max) > > CASE_TEST(mmap_munmap_good); EXPECT_SYSZR(1, test_mmap_munmap()); break; > > CASE_TEST(open_tty); EXPECT_SYSNE(1, tmp = open("/dev/null", 0), -1); if (tmp != -1) close(tmp); break; > > CASE_TEST(open_blah); EXPECT_SYSER(1, tmp = open("/proc/self/blah", 0), -1, ENOENT); if (tmp != -1) close(tmp); break; > > + CASE_TEST(openat_dir); EXPECT_SYSNE(1, test_openat(), -1); break; > > EXPECT_SYSZR() should work here. > > > CASE_TEST(pipe); EXPECT_SYSZR(1, test_pipe()); break; > > CASE_TEST(poll_null); EXPECT_SYSZR(1, poll(NULL, 0, 0)); break; > > CASE_TEST(poll_stdout); EXPECT_SYSNE(1, ({ struct pollfd fds = { 1, POLLOUT, 0}; poll(&fds, 1, 0); }), -1); break; > > -- > > 2.45.2 > >
diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h index 8f44c33b1213..3cd938f9abda 100644 --- a/tools/include/nolibc/sys.h +++ b/tools/include/nolibc/sys.h @@ -765,6 +765,31 @@ int mount(const char *src, const char *tgt, return __sysret(sys_mount(src, tgt, fst, flags, data)); } +/* + * int openat(int dirfd, const char *path, int flags[, mode_t mode]); + */ + +static __attribute__((unused)) +int sys_openat(int dirfd, const char *path, int flags, mode_t mode) +{ + return my_syscall4(__NR_openat, dirfd, path, flags, mode); +} + +static __attribute__((unused)) +int openat(int dirfd, const char *path, int flags, ...) +{ + mode_t mode = 0; + + if (flags & O_CREAT) { + va_list args; + + va_start(args, flags); + mode = va_arg(args, mode_t); + va_end(args); + } + + return __sysret(sys_openat(dirfd, path, flags, mode)); +} /* * int open(const char *path, int flags[, mode_t mode]); diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 79c3e6a845f3..2a1629938dd6 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -1028,6 +1028,26 @@ int test_rlimit(void) return 0; } +static int test_openat(void) +{ + int dev; + int null; + + dev = openat(AT_FDCWD, "/dev", O_DIRECTORY); + if (dev < 0) + return -1; + + null = openat(dev, "null", 0); + if (null < 0) { + close(dev); + return -1; + } + + close(dev); + close(null); + + return 0; +} /* Run syscall tests between IDs <min> and <max>. * Return 0 on success, non-zero on failure. @@ -1116,6 +1136,7 @@ int run_syscall(int min, int max) CASE_TEST(mmap_munmap_good); EXPECT_SYSZR(1, test_mmap_munmap()); break; CASE_TEST(open_tty); EXPECT_SYSNE(1, tmp = open("/dev/null", 0), -1); if (tmp != -1) close(tmp); break; CASE_TEST(open_blah); EXPECT_SYSER(1, tmp = open("/proc/self/blah", 0), -1, ENOENT); if (tmp != -1) close(tmp); break; + CASE_TEST(openat_dir); EXPECT_SYSNE(1, test_openat(), -1); break; CASE_TEST(pipe); EXPECT_SYSZR(1, test_pipe()); break; CASE_TEST(poll_null); EXPECT_SYSZR(1, poll(NULL, 0, 0)); break; CASE_TEST(poll_stdout); EXPECT_SYSNE(1, ({ struct pollfd fds = { 1, POLLOUT, 0}; poll(&fds, 1, 0); }), -1); break;
openat is useful to avoid needing to construct relative paths, so expose a wrapper for using it directly. Signed-off-by: Louis Taylor <louis@kragniz.eu> --- tools/include/nolibc/sys.h | 25 ++++++++++++++++++++ tools/testing/selftests/nolibc/nolibc-test.c | 21 ++++++++++++++++ 2 files changed, 46 insertions(+)