From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.4 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 4561 invoked from network); 12 Apr 2021 06:10:09 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 12 Apr 2021 06:10:09 -0000 Received: (qmail 17674 invoked by uid 550); 12 Apr 2021 06:10:02 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 17653 invoked from network); 12 Apr 2021 06:10:01 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=dereferenced.org; s=mailbun; t=1618207786; bh=LCJXxX/Jc49p0XMogFv0xMmsg0nAEI2gsywfbR4Im3Q=; h=Date:From:To:cc:Subject:In-Reply-To:References; b=cdnFAkBJJ8Bxu7jDsFf5/9flYYz89QDSdO4RGCgg2lB+GrDRvfXefl6mALmigLDiK w4ZiAR29SB1AyOiU/U8+oHIKIQS94E8MqcY7EBnqpRwyTBj4MUn4fDCQ5+zCNEfR0Q fU0MP6QSfQ+pIPPWhm3b+9/nQxUFiywzP7rDhzmH8AT1ieM06BWa1GWk3DKe4PXnNb 2ThPNNMYv3TjFJ0U8UqPTP8PbaFPLTkJnUXh0vpp0I7gLIPPgTzWM+9yCcxn8pdrYK JnXl/0aEYrWnXdn8fJU/w/LJoB9pRg3KUc2kcUZMn+3cZd8ZVYV96HKlkd2lwbXRym I2p7v/Yfcsd4g== Date: Mon, 12 Apr 2021 00:09:47 -0600 (MDT) From: Ariadne Conill To: musl@lists.openwall.com cc: Dominique Martinet In-Reply-To: <20210326054456.899700-1-dominique.martinet@atmark-techno.com> Message-ID: <70ea894a-a4bf-1ae8-ddce-1393caab9b@dereferenced.org> References: <20210326054456.899700-1-dominique.martinet@atmark-techno.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Subject: Re: [musl] [PATCH] nftw: implement FTW_ACTIONRETVAL 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 > #include > #include > > 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 > #include > > +#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