mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] [PATCH v3 0/2] nftw: Implement FTW_CHDIR and FTW_ACTIONRETVAL
@ 2022-01-23 15:59 Ismael Luceno
  2022-01-23 15:59 ` [musl] [PATCH v3 1/2] nftw: implement FTW_CHDIR Ismael Luceno
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Ismael Luceno @ 2022-01-23 15:59 UTC (permalink / raw)
  To: musl; +Cc: Rich Felker, Ismael Luceno

FTW_CHDIR is specified by POSIX. While the spec doesn't clarify, nftw
restores the starting directory in conformance with other implementations.

FTW_ACTIONRETVAL is a GNU extension added by glibc 2.3.3.

Changes since v2:

- Fixed error handling on FTW_CHDIR

Changes since v1:

- Fixed handling of FTW_STOP when FTW_DEPTH isn't set.

Ismael Luceno (2):
  nftw: implement FTW_CHDIR
  nftw: implement FTW_ACTIONRETVAL (GNU extension)

 include/ftw.h   |  7 +++++++
 src/misc/nftw.c | 55 ++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 52 insertions(+), 10 deletions(-)

-- 
2.33.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [musl] [PATCH v3 1/2] nftw: implement FTW_CHDIR
  2022-01-23 15:59 [musl] [PATCH v3 0/2] nftw: Implement FTW_CHDIR and FTW_ACTIONRETVAL Ismael Luceno
@ 2022-01-23 15:59 ` Ismael Luceno
  2022-05-12 13:00   ` Rich Felker
  2022-01-23 15:59 ` [musl] [PATCH v3 2/2] nftw: implement FTW_ACTIONRETVAL (GNU extension) Ismael Luceno
  2022-01-24 22:13 ` [musl] [PATCH v3 0/2] nftw: Implement FTW_CHDIR and FTW_ACTIONRETVAL Rich Felker
  2 siblings, 1 reply; 10+ messages in thread
From: Ismael Luceno @ 2022-01-23 15:59 UTC (permalink / raw)
  To: musl; +Cc: Rich Felker, Ismael Luceno

Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
---
 src/misc/nftw.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/src/misc/nftw.c b/src/misc/nftw.c
index 5b233b2b8e77..7569a657e54e 100644
--- a/src/misc/nftw.c
+++ b/src/misc/nftw.c
@@ -87,6 +87,14 @@ static int do_nftw(char *path, int (*fn)(const char *, const struct stat *, int,
 		DIR *d = fdopendir(dfd);
 		if (d) {
 			struct dirent *de;
+			if (flags & FTW_CHDIR) {
+				if (!fchdir(dfd)) {
+					err = errno;
+					closedir(d);
+					errno = err;
+					return -1;
+				}
+			}
 			while ((de = readdir(d))) {
 				if (de->d_name[0] == '.'
 				 && (!de->d_name[1]
@@ -123,6 +131,7 @@ int nftw(const char *path, int (*fn)(const char *, const struct stat *, int, str
 	int r, cs;
 	size_t l;
 	char pathbuf[PATH_MAX+1];
+	int orig_dfd;
 
 	if (fd_limit <= 0) return 0;
 
@@ -133,9 +142,22 @@ int nftw(const char *path, int (*fn)(const char *, const struct stat *, int, str
 	}
 	memcpy(pathbuf, path, l+1);
 	
+	if (flags & FTW_CHDIR) {
+		orig_dfd = open(".", O_CLOEXEC | O_PATH);
+		if (orig_dfd < 0)
+			return -1;
+	}
+
 	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
 	r = do_nftw(pathbuf, fn, fd_limit, flags, NULL);
 	pthread_setcancelstate(cs, 0);
+	if (flags & FTW_CHDIR) {
+		if (!fchdir(orig_dfd))
+			r = -1;
+		int err = errno;
+		close(orig_dfd);
+		errno = err;
+	}
 	return r;
 }
 
-- 
2.33.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [musl] [PATCH v3 2/2] nftw: implement FTW_ACTIONRETVAL (GNU extension)
  2022-01-23 15:59 [musl] [PATCH v3 0/2] nftw: Implement FTW_CHDIR and FTW_ACTIONRETVAL Ismael Luceno
  2022-01-23 15:59 ` [musl] [PATCH v3 1/2] nftw: implement FTW_CHDIR Ismael Luceno
@ 2022-01-23 15:59 ` Ismael Luceno
  2022-01-24  0:47   ` Dominique MARTINET
  2022-01-24 22:13 ` [musl] [PATCH v3 0/2] nftw: Implement FTW_CHDIR and FTW_ACTIONRETVAL Rich Felker
  2 siblings, 1 reply; 10+ messages in thread
From: Ismael Luceno @ 2022-01-23 15:59 UTC (permalink / raw)
  To: musl; +Cc: Rich Felker, Ismael Luceno

Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
---
 include/ftw.h   |  7 +++++++
 src/misc/nftw.c | 33 +++++++++++++++++++++++----------
 2 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/include/ftw.h b/include/ftw.h
index b15c062a8389..2e77dc76e11b 100644
--- a/include/ftw.h
+++ b/include/ftw.h
@@ -21,6 +21,13 @@ extern "C" {
 #define FTW_CHDIR 4
 #define FTW_DEPTH 8
 
+#define FTW_ACTIONRETVAL  16
+/* return values for callback */
+#define FTW_CONTINUE	  0
+#define FTW_STOP          1
+#define FTW_SKIP_SUBTREE  2
+#define FTW_SKIP_SIBLINGS 3
+
 struct FTW {
 	int base;
 	int level;
diff --git a/src/misc/nftw.c b/src/misc/nftw.c
index 7569a657e54e..382169f66b8b 100644
--- a/src/misc/nftw.c
+++ b/src/misc/nftw.c
@@ -26,7 +26,7 @@ static int do_nftw(char *path, int (*fn)(const char *, const struct stat *, int,
 	struct stat st;
 	struct history new;
 	int type;
-	int r;
+	int r, r2;
 	int dfd;
 	int err;
 	struct FTW lev;
@@ -72,12 +72,19 @@ static int do_nftw(char *path, int (*fn)(const char *, const struct stat *, int,
 		if (!fd_limit) close(dfd);
 	}
 
-	if (!(flags & FTW_DEPTH) && (r=fn(path, &st, type, &lev)))
-		return r;
+	r = 0;
+	if (!(flags & FTW_DEPTH) && (r=fn(path, &st, type, &lev))) {
+		if ((flags & FTW_ACTIONRETVAL)) {
+			if (r == FTW_STOP) return FTW_STOP;
+			if (r == FTW_SKIP_SUBTREE) return 0;
+			/* other values are saved for when returning */
+		} else
+			return r;
+	}
 
 	for (; h; h = h->chain)
 		if (h->dev == st.st_dev && h->ino == st.st_ino)
-			return 0;
+			return r;
 
 	if ((type == FTW_D || type == FTW_DP) && fd_limit) {
 		if (dfd < 0) {
@@ -107,9 +114,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 ((r2=do_nftw(path, fn, fd_limit-1, flags, &new))) {
+					if ((flags & FTW_ACTIONRETVAL)
+					    && r2 == FTW_SKIP_SIBLINGS)
+						break;
 					closedir(d);
-					return r;
+					return r2;
 				}
 			}
 			closedir(d);
@@ -120,10 +130,13 @@ static int do_nftw(char *path, int (*fn)(const char *, const struct stat *, int,
 	}
 
 	path[l] = 0;
-	if ((flags & FTW_DEPTH) && (r=fn(path, &st, type, &lev)))
-		return r;
-
-	return 0;
+	if (flags & FTW_DEPTH) {
+		r = fn(path, &st, type, &lev);
+		/* ignore FTW_SKIP_SUBTREE (too late), the caller is broken */
+		if ((flags & FTW_ACTIONRETVAL) && r == FTW_SKIP_SUBTREE)
+			return 0;
+	}
+	return r;
 }
 
 int nftw(const char *path, int (*fn)(const char *, const struct stat *, int, struct FTW *), int fd_limit, int flags)
-- 
2.33.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] [PATCH v3 2/2] nftw: implement FTW_ACTIONRETVAL (GNU extension)
  2022-01-23 15:59 ` [musl] [PATCH v3 2/2] nftw: implement FTW_ACTIONRETVAL (GNU extension) Ismael Luceno
@ 2022-01-24  0:47   ` Dominique MARTINET
  2022-01-24 19:53     ` Ismael Luceno
  0 siblings, 1 reply; 10+ messages in thread
From: Dominique MARTINET @ 2022-01-24  0:47 UTC (permalink / raw)
  To: musl; +Cc: Rich Felker, Ismael Luceno

This didn't get much traction when I submitted one last year:
https://www.openwall.com/lists/musl/2021/03/26/1
(and there were at least a couple other occurences I could find at the
time)
But given it keeps getting resubmitted I assume we can say that confirms
there's demand for it?

Ismael Luceno wrote on Sun, Jan 23, 2022 at 04:59:55PM +0100:
> diff --git a/include/ftw.h b/include/ftw.h
> index b15c062a8389..2e77dc76e11b 100644
> --- a/include/ftw.h
> +++ b/include/ftw.h
> @@ -21,6 +21,13 @@ extern "C" {
>  #define FTW_CHDIR 4
>  #define FTW_DEPTH 8
>  
> +#define FTW_ACTIONRETVAL  16
> +/* return values for callback */
> +#define FTW_CONTINUE	  0
> +#define FTW_STOP          1
> +#define FTW_SKIP_SUBTREE  2
> +#define FTW_SKIP_SIBLINGS 3

FWIW since this is a GNU extension I only had defined these in a
#ifdef _GNU_SOURCE block, but this had the ugly side effect of having to
redefine SUBTREE/SIBLINGS in the .c so I'm not sure which is better.

> +
>  struct FTW {
>  	int base;
>  	int level;
> diff --git a/src/misc/nftw.c b/src/misc/nftw.c
> index 7569a657e54e..382169f66b8b 100644
> --- a/src/misc/nftw.c
> +++ b/src/misc/nftw.c
> @@ -26,7 +26,7 @@ static int do_nftw(char *path, int (*fn)(const char *, const struct stat *, int,
>  	struct stat st;
>  	struct history new;
>  	int type;
> -	int r;
> +	int r, r2;
>  	int dfd;
>  	int err;
>  	struct FTW lev;
> @@ -72,12 +72,19 @@ static int do_nftw(char *path, int (*fn)(const char *, const struct stat *, int,
>  		if (!fd_limit) close(dfd);
>  	}
>  
> -	if (!(flags & FTW_DEPTH) && (r=fn(path, &st, type, &lev)))
> -		return r;
> +	r = 0;
> +	if (!(flags & FTW_DEPTH) && (r=fn(path, &st, type, &lev))) {
> +		if ((flags & FTW_ACTIONRETVAL)) {
> +			if (r == FTW_STOP) return FTW_STOP;
> +			if (r == FTW_SKIP_SUBTREE) return 0;
> +			/* other values are saved for when returning */

Hm, I'd naively think you would want to return immediately the other
values as well, so the else below is wrong?
But I didn't take long enough to check what e.g. a SKIP_SIBLINGS would
mean here, the construction just looks a bit odd to me.

> +		} else
> +			return r;
> +	}
>  
>  	for (; h; h = h->chain)
>  		if (h->dev == st.st_dev && h->ino == st.st_ino)
> -			return 0;
> +			return r;
>  
>  	if ((type == FTW_D || type == FTW_DP) && fd_limit) {
>  		if (dfd < 0) {
> @@ -107,9 +114,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 ((r2=do_nftw(path, fn, fd_limit-1, flags, &new))) {
> +					if ((flags & FTW_ACTIONRETVAL)
> +					    && r2 == FTW_SKIP_SIBLINGS)
> +						break;
>  					closedir(d);
> -					return r;
> +					return r2;
>  				}
>  			}
>  			closedir(d);
> @@ -120,10 +130,13 @@ static int do_nftw(char *path, int (*fn)(const char *, const struct stat *, int,
>  	}
>  
>  	path[l] = 0;
> -	if ((flags & FTW_DEPTH) && (r=fn(path, &st, type, &lev)))
> -		return r;
> -
> -	return 0;
> +	if (flags & FTW_DEPTH) {
> +		r = fn(path, &st, type, &lev);
> +		/* ignore FTW_SKIP_SUBTREE (too late), the caller is broken */
> +		if ((flags & FTW_ACTIONRETVAL) && r == FTW_SKIP_SUBTREE)
> +			return 0;

IIRC the glibc implementation also ignores FTW_SKIP_SIBLINGS in that
case (nftw returns 0), I'm not sure how much of a 1-to-1 implementation
we want -- I had implemented my version through a black-box approach
with a client exercising all kind of different code paths as for a gnu
extension I'd assume glibc to be the reference.

I haven't taken the time to rerun that comparison (the test client is in
a comment in my patch), but I probably will if this looks like it would
get merged unless someone else did.

> +	}
> +	return r;
>  }
>  
>  int nftw(const char *path, int (*fn)(const char *, const struct stat *, int, struct FTW *), int fd_limit, int flags)

-- 
Dominique

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] [PATCH v3 2/2] nftw: implement FTW_ACTIONRETVAL (GNU extension)
  2022-01-24  0:47   ` Dominique MARTINET
@ 2022-01-24 19:53     ` Ismael Luceno
  2022-03-09  4:37       ` Dominique MARTINET
  0 siblings, 1 reply; 10+ messages in thread
From: Ismael Luceno @ 2022-01-24 19:53 UTC (permalink / raw)
  To: musl; +Cc: Rich Felker

On 24/Jan/2022 09:47, Dominique MARTINET wrote:
> This didn't get much traction when I submitted one last year:
> https://www.openwall.com/lists/musl/2021/03/26/1
> (and there were at least a couple other occurences I could find at the
> time)

Thanks for reviewing. I was unaware of your submission.

> But given it keeps getting resubmitted I assume we can say that confirms
> there's demand for it?
<...> 
> > @@ -72,12 +72,19 @@ static int do_nftw(char *path, int (*fn)(const char *, const struct stat *, int,
> >  		if (!fd_limit) close(dfd);
> >  	}
> >  
> > -	if (!(flags & FTW_DEPTH) && (r=fn(path, &st, type, &lev)))
> > -		return r;
> > +	r = 0;
> > +	if (!(flags & FTW_DEPTH) && (r=fn(path, &st, type, &lev))) {
> > +		if ((flags & FTW_ACTIONRETVAL)) {
> > +			if (r == FTW_STOP) return FTW_STOP;
> > +			if (r == FTW_SKIP_SUBTREE) return 0;
> > +			/* other values are saved for when returning */
> 
> Hm, I'd naively think you would want to return immediately the other
> values as well, so the else below is wrong?
> But I didn't take long enough to check what e.g. a SKIP_SIBLINGS would
> mean here, the construction just looks a bit odd to me.

SKIP_SIBLINGS doesn't imply SKIP_SUBTREE, so must be saved and returned
when finishing.

> > @@ -120,10 +130,13 @@ static int do_nftw(char *path, int (*fn)(const char *, const struct stat *, int,
> >  	}
> >  
> >  	path[l] = 0;
> > -	if ((flags & FTW_DEPTH) && (r=fn(path, &st, type, &lev)))
> > -		return r;
> > -
> > -	return 0;
> > +	if (flags & FTW_DEPTH) {
> > +		r = fn(path, &st, type, &lev);
> > +		/* ignore FTW_SKIP_SUBTREE (too late), the caller is broken */
> > +		if ((flags & FTW_ACTIONRETVAL) && r == FTW_SKIP_SUBTREE)
> > +			return 0;
> 
> IIRC the glibc implementation also ignores FTW_SKIP_SIBLINGS in that
> case (nftw returns 0), I'm not sure how much of a 1-to-1 implementation
> we want -- I had implemented my version through a black-box approach
> with a client exercising all kind of different code paths as for a gnu
> extension I'd assume glibc to be the reference.

FTW_SKIP_SUBTREE makes no sense with FTW_DEPTH, but FTW_SKIP_SIBLINGS
works with both.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] [PATCH v3 0/2] nftw: Implement FTW_CHDIR and FTW_ACTIONRETVAL
  2022-01-23 15:59 [musl] [PATCH v3 0/2] nftw: Implement FTW_CHDIR and FTW_ACTIONRETVAL Ismael Luceno
  2022-01-23 15:59 ` [musl] [PATCH v3 1/2] nftw: implement FTW_CHDIR Ismael Luceno
  2022-01-23 15:59 ` [musl] [PATCH v3 2/2] nftw: implement FTW_ACTIONRETVAL (GNU extension) Ismael Luceno
@ 2022-01-24 22:13 ` Rich Felker
  2 siblings, 0 replies; 10+ messages in thread
From: Rich Felker @ 2022-01-24 22:13 UTC (permalink / raw)
  To: musl

On Sun, Jan 23, 2022 at 04:59:53PM +0100, Ismael Luceno wrote:
> FTW_CHDIR is specified by POSIX. While the spec doesn't clarify, nftw
> restores the starting directory in conformance with other implementations.
> 
> FTW_ACTIONRETVAL is a GNU extension added by glibc 2.3.3.
> 
> Changes since v2:
> 
> - Fixed error handling on FTW_CHDIR
> 
> Changes since v1:
> 
> - Fixed handling of FTW_STOP when FTW_DEPTH isn't set.
> 
> Ismael Luceno (2):
>   nftw: implement FTW_CHDIR
>   nftw: implement FTW_ACTIONRETVAL (GNU extension)
> 
>  include/ftw.h   |  7 +++++++
>  src/misc/nftw.c | 55 ++++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 52 insertions(+), 10 deletions(-)
> 
> -- 
> 2.33.0

For future reference, a single email with a patch series as
attachments is preferable over the LKML thread wackiness and makes it
a lot easier to review and submit new versions as replies in the
existing thread rather than starting a new thread each time.

Rich

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] [PATCH v3 2/2] nftw: implement FTW_ACTIONRETVAL (GNU extension)
  2022-01-24 19:53     ` Ismael Luceno
@ 2022-03-09  4:37       ` Dominique MARTINET
  2022-03-09 11:41         ` Rich Felker
  0 siblings, 1 reply; 10+ messages in thread
From: Dominique MARTINET @ 2022-03-09  4:37 UTC (permalink / raw)
  To: Ismael Luceno, musl; +Cc: Rich Felker

Sorry for the delay, I was waiting for some "official" feedback and
forgot about the patch as none came.

Rich (or others), is there anything we can do to convince you the
feature would be interesting?
I had sent you recap from debian codesearch back in April last year:
https://www.openwall.com/lists/musl/2021/04/12/10
but didn't hear any reply.

It'd be great to just hear a final "no unless it gets more widely used"
so I could give up and work on converting bpftools to not depend on
it -- it's just frustrating due to the lack of reply, and while adding
FTW_ACTIONRETVAL to musl is easy converting a user to something else is
not so straightforward so I was putting it off...

(Conversely, if there is interest please say so as well -- my patch had
a couple of feedbacks I would be happy to address, but I just never sent
a v2 because it looked like it wouldn't be accepted anyway from your
(lack of) reaction. I don't care if my version of this one gets in
ultimately, and can help either way.)


Ismael, comments below.


Ismael Luceno wrote on Mon, Jan 24, 2022 at 08:53:14PM +0100:
> > > +	if (!(flags & FTW_DEPTH) && (r=fn(path, &st, type, &lev))) {
> > > +		if ((flags & FTW_ACTIONRETVAL)) {
> > > +			if (r == FTW_STOP) return FTW_STOP;
> > > +			if (r == FTW_SKIP_SUBTREE) return 0;
> > > +			/* other values are saved for when returning */
> > 
> > Hm, I'd naively think you would want to return immediately the other
> > values as well, so the else below is wrong?
> > But I didn't take long enough to check what e.g. a SKIP_SIBLINGS would
> > mean here, the construction just looks a bit odd to me.
> 
> SKIP_SIBLINGS doesn't imply SKIP_SUBTREE, so must be saved and returned
> when finishing.

Hm, I was starting to agree with you and wondered how my patch worked,
but actually... According to how glibc behaves, it does:

----
# in musl source tree after patch
$ make -j6
make: Nothing to be done for 'all'.
$ cat ftw/ftw.c
#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") == 0) {
                return FTW_SKIP_SIBLINGS;
        }
        return 0;
}

int main() {
        int rc = nftw(".", cb, 10, FTW_ACTIONRETVAL);
        printf("rc %d\n", rc);
        return 0;
}
$ gcc -o ftw/ftw_glibc ftw/ftw.c 
$ gcc -o ftw/ftw_musl ftw/ftw.c lib/libc.a
$ diff -u <( ./ftw/ftw_musl ) <( ./ftw/ftw_glibc )
--- /dev/fd/63	2022-03-09 13:22:55.566285599 +0900
+++ /dev/fd/62	2022-03-09 13:22:55.566285599 +0900
@@ -1,120 +1,3 @@
 got .
 got ./.git
-got ./.git/branches
-got ./.git/hooks
-got ./.git/hooks/applypatch-msg.sample
-got ./.git/hooks/commit-msg.sample
-got ./.git/hooks/fsmonitor-watchman.sample
...
-got ./.git/ORIG_HEAD
-got ./.git/index
-got ./.git/HEAD
 rc 0
----

so the glibc version of ftw skipped .git here when it only returned
FTW_SKIP_SIBLINGS.

You could argue that this is a bug in glibc, but since there is no spec
for FTW_ACTIONRETVAL I had chosen at the time to implement something
compatible bug-for-bug as "glibc is the reference".


> > >  	path[l] = 0;
> > > -	if ((flags & FTW_DEPTH) && (r=fn(path, &st, type, &lev)))
> > > -		return r;
> > > -
> > > -	return 0;
> > > +	if (flags & FTW_DEPTH) {
> > > +		r = fn(path, &st, type, &lev);
> > > +		/* ignore FTW_SKIP_SUBTREE (too late), the caller is broken */
> > > +		if ((flags & FTW_ACTIONRETVAL) && r == FTW_SKIP_SUBTREE)
> > > +			return 0;
> > 
> > IIRC the glibc implementation also ignores FTW_SKIP_SIBLINGS in that
> > case (nftw returns 0), I'm not sure how much of a 1-to-1 implementation
> > we want -- I had implemented my version through a black-box approach
> > with a client exercising all kind of different code paths as for a gnu
> > extension I'd assume glibc to be the reference.
> 
> FTW_SKIP_SUBTREE makes no sense with FTW_DEPTH, but FTW_SKIP_SIBLINGS
> works with both.

I agree it makes no sense, this was another bug-for-bug thing:

----
$ cat subtree.c 
#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) {
	return FTW_SKIP_SIBLINGS;
}

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

My version at the time just reset the return value at the end if
FTW_ACTIONRETVAL was specified in flags, to conform to that.

-- 
Dominique

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] [PATCH v3 2/2] nftw: implement FTW_ACTIONRETVAL (GNU extension)
  2022-03-09  4:37       ` Dominique MARTINET
@ 2022-03-09 11:41         ` Rich Felker
  2022-04-28  2:02           ` Dominique MARTINET
  0 siblings, 1 reply; 10+ messages in thread
From: Rich Felker @ 2022-03-09 11:41 UTC (permalink / raw)
  To: Dominique MARTINET; +Cc: Ismael Luceno, musl

On Wed, Mar 09, 2022 at 01:37:53PM +0900, Dominique MARTINET wrote:
> Sorry for the delay, I was waiting for some "official" feedback and
> forgot about the patch as none came.
> 
> Rich (or others), is there anything we can do to convince you the
> feature would be interesting?
> I had sent you recap from debian codesearch back in April last year:
> https://www.openwall.com/lists/musl/2021/04/12/10
> but didn't hear any reply.
> 
> It'd be great to just hear a final "no unless it gets more widely used"
> so I could give up and work on converting bpftools to not depend on
> it -- it's just frustrating due to the lack of reply, and while adding
> FTW_ACTIONRETVAL to musl is easy converting a user to something else is
> not so straightforward so I was putting it off...
> 
> (Conversely, if there is interest please say so as well -- my patch had
> a couple of feedbacks I would be happy to address, but I just never sent
> a v2 because it looked like it wouldn't be accepted anyway from your
> (lack of) reaction. I don't care if my version of this one gets in
> ultimately, and can help either way.)

I don't recall whether my question original about meeting criteria for
inclusion/exclusion was really addressed. I'll look back through the
mails and if so then yes this is probably waiting on feedback from me.

Rich

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] [PATCH v3 2/2] nftw: implement FTW_ACTIONRETVAL (GNU extension)
  2022-03-09 11:41         ` Rich Felker
@ 2022-04-28  2:02           ` Dominique MARTINET
  0 siblings, 0 replies; 10+ messages in thread
From: Dominique MARTINET @ 2022-04-28  2:02 UTC (permalink / raw)
  To: Rich Felker; +Cc: Ismael Luceno, musl

Hi Rich, Ismael,

Rich Felker wrote on Wed, Mar 09, 2022 at 06:41:15AM -0500:
> On Wed, Mar 09, 2022 at 01:37:53PM +0900, Dominique MARTINET wrote:
> > Rich (or others), is there anything we can do to convince you the
> > feature would be interesting?
> > I had sent you recap from debian codesearch back in April last year:
> > https://www.openwall.com/lists/musl/2021/04/12/10
> > but didn't hear any reply.
> > 
> > It'd be great to just hear a final "no unless it gets more widely used"
> > so I could give up and work on converting bpftools to not depend on
> > it -- it's just frustrating due to the lack of reply, and while adding
> > FTW_ACTIONRETVAL to musl is easy converting a user to something else is
> > not so straightforward so I was putting it off...
> > 
> > (Conversely, if there is interest please say so as well -- my patch had
> > a couple of feedbacks I would be happy to address, but I just never sent
> > a v2 because it looked like it wouldn't be accepted anyway from your
> > (lack of) reaction. I don't care if my version of this one gets in
> > ultimately, and can help either way.)
> 
> I don't recall whether my question original about meeting criteria for
> inclusion/exclusion was really addressed. I'll look back through the
> mails and if so then yes this is probably waiting on feedback from me.

For followup on my usecase, I've instead adapted bpftool to no longer
require FTW_ACTIONRETVAL (this specific call to nftw was replaced with
two nested readdir() loop), so bpftool now builds with stock musl
without this.

If there is ever interest again I'll be happy to finish polishing up any
implementation for this as I think it's quite handy, but otherwise I'll
just forget about it.


Thanks,
-- 
Dominique

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] [PATCH v3 1/2] nftw: implement FTW_CHDIR
  2022-01-23 15:59 ` [musl] [PATCH v3 1/2] nftw: implement FTW_CHDIR Ismael Luceno
@ 2022-05-12 13:00   ` Rich Felker
  0 siblings, 0 replies; 10+ messages in thread
From: Rich Felker @ 2022-05-12 13:00 UTC (permalink / raw)
  To: Ismael Luceno; +Cc: musl

On Sun, Jan 23, 2022 at 04:59:54PM +0100, Ismael Luceno wrote:
> Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
> ---
>  src/misc/nftw.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/src/misc/nftw.c b/src/misc/nftw.c
> index 5b233b2b8e77..7569a657e54e 100644
> --- a/src/misc/nftw.c
> +++ b/src/misc/nftw.c
> @@ -87,6 +87,14 @@ static int do_nftw(char *path, int (*fn)(const char *, const struct stat *, int,
>  		DIR *d = fdopendir(dfd);
>  		if (d) {
>  			struct dirent *de;
> +			if (flags & FTW_CHDIR) {
> +				if (!fchdir(dfd)) {
> +					err = errno;
> +					closedir(d);
> +					errno = err;
> +					return -1;
> +				}
> +			}
>  			while ((de = readdir(d))) {
>  				if (de->d_name[0] == '.'
>  				 && (!de->d_name[1]

I'm not sure how this works -- after the fchdir, subsequent open at
line 69 will fail because the pathname is no longer valid relative to
the new working directory. I think different logic is needed at that
point too in order to adjust the argument to open depending on
FTW_CHDIR (or the whole thing could be converted to using at
functions, but that's a much bigger change I'd really only want to do
in a full overhaul of this function). I'm guessing you only tested
with absolute pathnames.

> @@ -123,6 +131,7 @@ int nftw(const char *path, int (*fn)(const char *, const struct stat *, int, str
>  	int r, cs;
>  	size_t l;
>  	char pathbuf[PATH_MAX+1];
> +	int orig_dfd;
>  
>  	if (fd_limit <= 0) return 0;
>  
> @@ -133,9 +142,22 @@ int nftw(const char *path, int (*fn)(const char *, const struct stat *, int, str
>  	}
>  	memcpy(pathbuf, path, l+1);
>  	
> +	if (flags & FTW_CHDIR) {
> +		orig_dfd = open(".", O_CLOEXEC | O_PATH);
> +		if (orig_dfd < 0)
> +			return -1;
> +	}
> +
>  	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
>  	r = do_nftw(pathbuf, fn, fd_limit, flags, NULL);
>  	pthread_setcancelstate(cs, 0);
> +	if (flags & FTW_CHDIR) {
> +		if (!fchdir(orig_dfd))
> +			r = -1;
> +		int err = errno;
> +		close(orig_dfd);
> +		errno = err;
> +	}
>  	return r;
>  }
>  
> -- 
> 2.33.0

The added open and close calls need to be inside the range where
cancellation is blocked. Otherwise they will cause nftw to act on
cancellation.

Rich

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-05-12 13:00 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-23 15:59 [musl] [PATCH v3 0/2] nftw: Implement FTW_CHDIR and FTW_ACTIONRETVAL Ismael Luceno
2022-01-23 15:59 ` [musl] [PATCH v3 1/2] nftw: implement FTW_CHDIR Ismael Luceno
2022-05-12 13:00   ` Rich Felker
2022-01-23 15:59 ` [musl] [PATCH v3 2/2] nftw: implement FTW_ACTIONRETVAL (GNU extension) Ismael Luceno
2022-01-24  0:47   ` Dominique MARTINET
2022-01-24 19:53     ` Ismael Luceno
2022-03-09  4:37       ` Dominique MARTINET
2022-03-09 11:41         ` Rich Felker
2022-04-28  2:02           ` Dominique MARTINET
2022-01-24 22:13 ` [musl] [PATCH v3 0/2] nftw: Implement FTW_CHDIR and FTW_ACTIONRETVAL Rich Felker

Code repositories for project(s) associated with this inbox:

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).