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.3 required=5.0 tests=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 8286 invoked from network); 12 Apr 2021 06:45:36 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 12 Apr 2021 06:45:36 -0000 Received: (qmail 2038 invoked by uid 550); 12 Apr 2021 06:45:34 -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 2014 invoked from network); 12 Apr 2021 06:45:33 -0000 X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=mrnaVBBKptLHaV1gH4vXieMuMmrgSb98oDHlmKjPZF0=; b=tbdEOjNNSqql03GtfZJk/X7XemG3qoaHMwQF46Vt/CQGDp42fooE9Fs5lLkI5oHJxt VwjG4vB5axNaobDpvu4L+4WyVr/D0D27G6V+Lr5AdAI+SMujSj6CEKrZx/gUvGZCXLXu /fdMn1TNWsPt+M3zVh02s47ST4fWY8kdc1ORuhzzGszk0tvvi+6/f3o6K153kWGFIW/q sjZeX2lkIjNI8VZihlgPH5oHyq5btpsd05wsE+AhbTV6AmHeRleFozbnZSyVaueaaUhh 6j68LUi+SFZnCvHesxom4OFySmP/Eahs4re1M09pqSipS9NIC9ouqLQtuL4CN4EJDf+L sGXA== X-Gm-Message-State: AOAM532XerBTCRIB9qozBwXX0YqJSgkmS6tRIkycNkAWjzwSEvCYKrPu /V1B625xVKVGVj59TX6bHLZyztVBxAUmoBuoiDqw3gp43S1UrdjTX/l96+ohP4fQs5FXFX6G9Ux TxCVf/IV2vslL1hL0VD/vLw== X-Received: by 2002:a63:1e1e:: with SMTP id e30mr24823960pge.77.1618209919496; Sun, 11 Apr 2021 23:45:19 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzZYKe4KrA8SYoXFKJib7DHgTDqTVNfOKKXMaYo/UrFZmxvo3j16fy58FeNFnXLCZq4E/To5Q== X-Received: by 2002:a63:1e1e:: with SMTP id e30mr24823935pge.77.1618209919194; Sun, 11 Apr 2021 23:45:19 -0700 (PDT) Date: Mon, 12 Apr 2021 15:45:07 +0900 From: Dominique Martinet To: Ariadne Conill Cc: musl@lists.openwall.com Message-ID: References: <20210326054456.899700-1-dominique.martinet@atmark-techno.com> <70ea894a-a4bf-1ae8-ddce-1393caab9b@dereferenced.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <70ea894a-a4bf-1ae8-ddce-1393caab9b@dereferenced.org> Subject: Re: [musl] [PATCH] nftw: implement FTW_ACTIONRETVAL 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 > > #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. 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