[musl] [PATCH] nftw: implement FTW_ACTIONRETVAL

[musl] [PATCH] nftw: implement FTW_ACTIONRETVAL

[Dominique Martinet]

nftw is sometimes used with e.g. FTW_SKIP_SUBTREE on linux which
requires special handling.

A similar patch was sent in 2018[1] and general feedbacks for nftw
seemed positive, but the original work's license was not clear so this
implementation was made without using it, looking at the linux man page
and comparing with glibc's behaviour through test programs.

[1] https://www.openwall.com/lists/musl/2018/12/16/1
---
A few notes:
 - After checking there doesn't seem to be *that* many users, but there
still are quite a few, which can be found through debian code search:
https://codesearch.debian.net/search?q=FTW_SKIP_SUBTREE&literal=1&perpkg=1
I'm personally interested in this for bpftool (in linux's source tree,
tools/bpf/bpftool/perf.c ) which uses it to run through /proc/*/fd/*
skipping unrelated directories, on alpine linux.
If this is refused I'll try to push a workaround there but doing it in
musl would allow dropping other compat patches (e.g. aufs-tools) with
a similar problem.

 - I'm not happy that I had to copy the defines over, but I don't think
we can just define _GNU_SOURCE in nftw.c to get the values; if you have
any recommendation for this I would be happy to rework and test again

 - the man page isn't clear on what to do with SKIP_SUBTREE if the entry
is not a directory, but testing shows it is just ignored on glibc 2.31
so I didn't add any check. The code is simpler that way.

 - I looked into doing/modifying some test suite, but the only I could
find related to musl (libc-test) does not perform any runtime check on
nftw, so all my tests were just manually adjusting values and comparing.

Here is a trivial test program if that helps anyone:
---
#define _GNU_SOURCE
#include <ftw.h>
#include <string.h>
#include <stdio.h>

int cb(const char *path, const struct stat *sb, int typeflag, struct FTW *ftwbuf) {
	printf("got %s\n", path);
	if (strcmp(path, "./src") == 0)
		return FTW_SKIP_SUBTREE;
	if (strcmp(path, "./.git/description") == 0) {
		return FTW_SKIP_SIBLINGS;
	}
	return 0;
}

int main() {
	int rc = nftw(".", cb, 10, FTW_ACTIONRETVAL);
	printf("rc %d\n", rc);
	return 0;
}
---

 include/ftw.h   |  8 ++++++++
 src/misc/nftw.c | 13 +++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/include/ftw.h b/include/ftw.h
index b15c062a8389..5b07855fefcc 100644
--- a/include/ftw.h
+++ b/include/ftw.h
@@ -21,6 +21,14 @@ extern "C" {
 #define FTW_CHDIR 4
 #define FTW_DEPTH 8
 
+#ifdef _GNU_SOURCE
+#define FTW_ACTIONRETVAL 0x10
+#define FTW_CONTINUE 0
+#define FTW_STOP 1
+#define FTW_SKIP_SUBTREE 2
+#define FTW_SKIP_SIBLINGS 3
+#endif
+
 struct FTW {
 	int base;
 	int level;
diff --git a/src/misc/nftw.c b/src/misc/nftw.c
index 8dcff7fefd2a..2994968dcbbe 100644
--- a/src/misc/nftw.c
+++ b/src/misc/nftw.c
@@ -8,6 +8,10 @@
 #include <limits.h>
 #include <pthread.h>
 
+#define FTW_ACTIONRETVAL 0x10
+#define FTW_SKIP_SUBTREE 2
+#define FTW_SKIP_SIBLINGS 3
+
 struct history
 {
 	struct history *chain;
@@ -100,6 +104,12 @@ static int do_nftw(char *path, int (*fn)(const char *, const struct stat *, int,
 				path[j]='/';
 				strcpy(path+j+1, de->d_name);
 				if ((r=do_nftw(path, fn, fd_limit-1, flags, &new))) {
+					if (flags & FTW_ACTIONRETVAL) {
+						if (r == FTW_SKIP_SIBLINGS)
+							break;
+						if (r == FTW_SKIP_SUBTREE)
+							continue;
+					}
 					closedir(d);
 					return r;
 				}
@@ -136,6 +146,9 @@ int nftw(const char *path, int (*fn)(const char *, const struct stat *, int, str
 	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
 	r = do_nftw(pathbuf, fn, fd_limit, flags, NULL);
 	pthread_setcancelstate(cs, 0);
+	if ((flags & FTW_ACTIONRETVAL)
+	 && (r == FTW_SKIP_SIBLINGS || r == FTW_SKIP_SUBTREE))
+		r = 0;
 	return r;
 }
 
-- 
2.30.2

[musl] Re: [PATCH] nftw: implement FTW_ACTIONRETVAL

[Dominique Martinet]

Hi,

Dominique Martinet wrote on Fri, Mar 26, 2021 at 02:44:56PM +0900:
>  - After checking there doesn't seem to be *that* many users, but there
> still are quite a few, which can be found through debian code search:
> https://codesearch.debian.net/search?q=FTW_SKIP_SUBTREE&literal=1&perpkg=1
> I'm personally interested in this for bpftool (in linux's source tree,
> tools/bpf/bpftool/perf.c ) which uses it to run through /proc/*/fd/*
> skipping unrelated directories, on alpine linux.
> If this is refused I'll try to push a workaround there but doing it in
> musl would allow dropping other compat patches (e.g. aufs-tools) with
> a similar problem.

It's been a couple of weeks.

I'm not sure how long people usually wait for replies, but I'll give
this one more then start looking at working around this on the bpftool
side for my own personal needs, but it's a bit of a shame for  other
users of FTW_ACTIONRETVAL that could benefit from this in my opinion :(


Thanks,
-- 
Dominique

Re: [musl] [PATCH] nftw: implement FTW_ACTIONRETVAL

[Ariadne Conill]

Hello,

I'm not an expert on the nftw APIs, so please be patient.

On Fri, 26 Mar 2021, Dominique Martinet wrote:

> nftw is sometimes used with e.g. FTW_SKIP_SUBTREE on linux which
> requires special handling.
>
> A similar patch was sent in 2018[1] and general feedbacks for nftw
> seemed positive, but the original work's license was not clear so this
> implementation was made without using it, looking at the linux man page
> and comparing with glibc's behaviour through test programs.
>
> [1] https://www.openwall.com/lists/musl/2018/12/16/1
> ---
> A few notes:
> - After checking there doesn't seem to be *that* many users, but there
> still are quite a few, which can be found through debian code search:
> https://codesearch.debian.net/search?q=FTW_SKIP_SUBTREE&literal=1&perpkg=1
> I'm personally interested in this for bpftool (in linux's source tree,
> tools/bpf/bpftool/perf.c ) which uses it to run through /proc/*/fd/*
> skipping unrelated directories, on alpine linux.
> If this is refused I'll try to push a workaround there but doing it in
> musl would allow dropping other compat patches (e.g. aufs-tools) with
> a similar problem.
>
> - I'm not happy that I had to copy the defines over, but I don't think
> we can just define _GNU_SOURCE in nftw.c to get the values; if you have
> any recommendation for this I would be happy to rework and test again
>
> - the man page isn't clear on what to do with SKIP_SUBTREE if the entry
> is not a directory, but testing shows it is just ignored on glibc 2.31
> so I didn't add any check. The code is simpler that way.
>
> - I looked into doing/modifying some test suite, but the only I could
> find related to musl (libc-test) does not perform any runtime check on
> nftw, so all my tests were just manually adjusting values and comparing.
>
> Here is a trivial test program if that helps anyone:
> ---
> #define _GNU_SOURCE
> #include <ftw.h>
> #include <string.h>
> #include <stdio.h>
>
> int cb(const char *path, const struct stat *sb, int typeflag, struct FTW *ftwbuf) {
> 	printf("got %s\n", path);
> 	if (strcmp(path, "./src") == 0)
> 		return FTW_SKIP_SUBTREE;
> 	if (strcmp(path, "./.git/description") == 0) {
> 		return FTW_SKIP_SIBLINGS;
> 	}
> 	return 0;
> }
>
> int main() {
> 	int rc = nftw(".", cb, 10, FTW_ACTIONRETVAL);
> 	printf("rc %d\n", rc);
> 	return 0;
> }
> ---
>
> include/ftw.h   |  8 ++++++++
> src/misc/nftw.c | 13 +++++++++++++
> 2 files changed, 21 insertions(+)
>
> diff --git a/include/ftw.h b/include/ftw.h
> index b15c062a8389..5b07855fefcc 100644
> --- a/include/ftw.h
> +++ b/include/ftw.h
> @@ -21,6 +21,14 @@ extern "C" {
> #define FTW_CHDIR 4
> #define FTW_DEPTH 8
>
> +#ifdef _GNU_SOURCE
> +#define FTW_ACTIONRETVAL 0x10
> +#define FTW_CONTINUE 0
> +#define FTW_STOP 1

It would be nice to document why FTW_CONTINUE and FTW_STOP were added as 
constants, since the original implementation did not use them.  I found 
myself quite confused, since the code below did not reference either of 
these constants and had to look up the spec for the nftw API.  This could 
be done in the commit message, it's just helpful for understanding the 
context.

> +#define FTW_SKIP_SUBTREE 2
> +#define FTW_SKIP_SIBLINGS 3
> +#endif
> +
> struct FTW {
> 	int base;
> 	int level;
> diff --git a/src/misc/nftw.c b/src/misc/nftw.c
> index 8dcff7fefd2a..2994968dcbbe 100644
> --- a/src/misc/nftw.c
> +++ b/src/misc/nftw.c
> @@ -8,6 +8,10 @@
> #include <limits.h>
> #include <pthread.h>
>
> +#define FTW_ACTIONRETVAL 0x10
> +#define FTW_SKIP_SUBTREE 2
> +#define FTW_SKIP_SIBLINGS 3
> +
> struct history
> {
> 	struct history *chain;
> @@ -100,6 +104,12 @@ static int do_nftw(char *path, int (*fn)(const char *, const struct stat *, int,
> 				path[j]='/';
> 				strcpy(path+j+1, de->d_name);
> 				if ((r=do_nftw(path, fn, fd_limit-1, flags, &new))) {
> +					if (flags & FTW_ACTIONRETVAL) {

Why not set r to zero here?  It would allow you to remove the next part 
entirely.

> +						if (r == FTW_SKIP_SIBLINGS)
> +							break;
> +						if (r == FTW_SKIP_SUBTREE)
> +							continue;
> +					}
> 					closedir(d);
> 					return r;
> 				}
> @@ -136,6 +146,9 @@ int nftw(const char *path, int (*fn)(const char *, const struct stat *, int, str
> 	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
> 	r = do_nftw(pathbuf, fn, fd_limit, flags, NULL);
> 	pthread_setcancelstate(cs, 0);
> +	if ((flags & FTW_ACTIONRETVAL)
> +	 && (r == FTW_SKIP_SIBLINGS || r == FTW_SKIP_SUBTREE))
> +		r = 0;
> 	return r;
> }

It seems mostly OK, except for the nitpicks I pointed out.

Ariadne

Re: [musl] [PATCH] nftw: implement FTW_ACTIONRETVAL

[Dominique Martinet]

Hi,

Ariadne Conill wrote on Mon, Apr 12, 2021 at 12:09:47AM -0600:
> I'm not an expert on the nftw APIs, so please be patient.

Thanks for the reply.
I'm in no particular hurry, as long as I know I'm not ignored :)

It's not always obvious on big lists, so sorry for my tone.

> > +#ifdef _GNU_SOURCE
> > +#define FTW_ACTIONRETVAL 0x10
> > +#define FTW_CONTINUE 0
> > +#define FTW_STOP 1
> 
> It would be nice to document why FTW_CONTINUE and FTW_STOP were added as
> constants, since the original implementation did not use them.  I found
> myself quite confused, since the code below did not reference either of
> these constants and had to look up the spec for the nftw API.  This could be
> done in the commit message, it's just helpful for understanding the context.

That's a good point, the code does not reference these because it just
somehow works out with these value, but I actually have no idea what to
use as the spec myself...
I based myself on the linux man pages[1], which only refers to various
versions of POSIX, but as far as I can see these do not define any spec
for FTW_ACTIONRETVAL -- so where is the source of truth? glibc?

[1] https://man7.org/linux/man-pages/man3/nftw.3.html

If you have something to use as reference I can add it to the commit
message for v2, yes.

> > diff --git a/src/misc/nftw.c b/src/misc/nftw.c
> > index 8dcff7fefd2a..2994968dcbbe 100644
> > --- a/src/misc/nftw.c
> > +++ b/src/misc/nftw.c
> > @@ -8,6 +8,10 @@
> > #include <limits.h>
> > #include <pthread.h>
> > 
> > +#define FTW_ACTIONRETVAL 0x10
> > +#define FTW_SKIP_SUBTREE 2
> > +#define FTW_SKIP_SIBLINGS 3
> > +
> > struct history
> > {
> > 	struct history *chain;
> > @@ -100,6 +104,12 @@ static int do_nftw(char *path, int (*fn)(const char *, const struct stat *, int,
> > 				path[j]='/';
> > 				strcpy(path+j+1, de->d_name);
> > 				if ((r=do_nftw(path, fn, fd_limit-1, flags, &new))) {
> > +					if (flags & FTW_ACTIONRETVAL) {
> 
> Why not set r to zero here?  It would allow you to remove the next part
> entirely.

I'm sorry, I'm not sure what 'the next part' refers to here.

If it's the two break/continue checks, the continue check could be be
skipped by making fn return value return 0 if retval is set and r was
FTW_SKIP_SUBTREE; but FTW_SKIP_SIBLINGS would have to stay, so the code
would be half simplified at the cost of adding extra checks for both fn
calls to fix the return value.

If it's the part in nftw itself (I guess that's what you meant),
then unfortunately that's not true if the function would return
SKIP_SIBLING or SKIP_SUBTREE for the top level do_nftw() call.
That's about just as corner case as it can get, but I implemented this
comparing what glibc returns for various corner cases and glibc's nftw()
returns 0 if the callback function returns FTW_SKIP_SIBLINGS or SUBTREE
on its first invocation... Which would no longer be the case for us,
hence the extra check.

I'm not sure how much musl cares about compat there, the man page does
not describe what nftw() should return if the callbacks returned these
requests.
I personally think asking to skip the first directory is borderline
invalid usage, but if we skip it nftw worked as intended so it's a
success and we should return 0? I guess... Well, no strong feeling
either way, happy to set r to 0 here (after the two checks) and skip
the last cleanu check in nftw().


Thanks,
-- 
Dominique

Re: [musl] [PATCH] nftw: implement FTW_ACTIONRETVAL

[Rich Felker]

On Fri, Mar 26, 2021 at 02:44:56PM +0900, Dominique Martinet wrote:
> nftw is sometimes used with e.g. FTW_SKIP_SUBTREE on linux which
> requires special handling.
> 
> A similar patch was sent in 2018[1] and general feedbacks for nftw
> seemed positive, but the original work's license was not clear so this
> implementation was made without using it, looking at the linux man page
> and comparing with glibc's behaviour through test programs.
> 
> [1] https://www.openwall.com/lists/musl/2018/12/16/1

Feedback for that submission as a whole was negative because it was
just "throw in everything I found that systemd wanted", which is a
policy discussion that's already been had and the answer is no.
However that's not a judgement on any individual feature like
FTW_ACTIONRETVAL, which could meet criteria for inclusion. I haven't
gotten a good chance to look at this yet, but if you haven't already,
could you post some supporting reasons for this (like widespread usage
by applications that you've found)? That would help determine if it's
appropriate.

Rich

Re: [musl] [PATCH] nftw: implement FTW_ACTIONRETVAL

[Dominique Martinet]

Rich Felker wrote on Mon, Apr 12, 2021 at 10:17:30AM -0400:
> On Fri, Mar 26, 2021 at 02:44:56PM +0900, Dominique Martinet wrote:
> > nftw is sometimes used with e.g. FTW_SKIP_SUBTREE on linux which
> > requires special handling.
> > 
> > A similar patch was sent in 2018[1] and general feedbacks for nftw
> > seemed positive, but the original work's license was not clear so this
> > implementation was made without using it, looking at the linux man page
> > and comparing with glibc's behaviour through test programs.
> > 
> > [1] https://www.openwall.com/lists/musl/2018/12/16/1
> 
> Feedback for that submission as a whole was negative because it was
> just "throw in everything I found that systemd wanted", which is a
> policy discussion that's already been had and the answer is no.

I agree the original submission was pretty bad, and without any
follow-up to justify individual features I would not have taken anything
from it either.


> However that's not a judgement on any individual feature like
> FTW_ACTIONRETVAL, which could meet criteria for inclusion. I haven't
> gotten a good chance to look at this yet, but if you haven't already,
> could you post some supporting reasons for this (like widespread usage
> by applications that you've found)? That would help determine if it's
> appropriate.

I posted a quick note about users of the feature in the patch comments:
---
 - After checking there doesn't seem to be *that* many users, but there
 still are quite a few, which can be found through debian code search:
 https://codesearch.debian.net/search?q=FTW_SKIP_SUBTREE&literal=1&perpkg=1
 I'm personally interested in this for bpftool (in linux's source tree,
 tools/bpf/bpftool/perf.c ) which uses it to run through /proc/*/fd/*
 skipping unrelated directories, on alpine linux.
---

I would find that pretty hard to call it "widespread usage" (there are
14 distinct packages that use the feature according to the debian
codesearch, 15 if you search for FTW_ACTIONRETVAL instead), probably
because there aren't that many users of ntfw in the first place -- fts*
seems to be more popular, and people who want to do complex things such
as skipping based on pattern might also be enclined to reimplement it as
it is fairly straightforward to... 

For alpine itself, the only main existing patched package I could find
to accomodate for FTW_ACTIONRETVAL was aufs-tools, where the patch just
reimplemented another nftw with the feature for it (it looks like they
also based off musl's implementation); merging this here would allow
dropping that patch and simplify the package maintenance a bit moving
forward.
(There is another in "loolwsd", where they just define the values to
compile but do not implement the behaviour!! So they probably do not
care too much...)

There are other packages from the list that are packaged in alpine, it
looks like for example samba has a baked-in workaround that just skips
trying to use FTW_ACTIONRETVAL if the value is not defined, I assume
they handle it gracefully and the result is just less performant code
but I did not look further.



All in all I just had a particular usecase in mind (linux's bpftool) and
selfishly picked the least intrusive change (reworking bpftool to use
something else would be slightly more changes), thinking at the time others
would benefit from it before checking thoroughly.
Now there is more data I am really open to both ways of thinking (this
should be implemented in musl vs. this should be worked around in
alpine) and just bumped the thread yesterday to move forward in the
later case.


Thanks,
-- 
Dominique