[PATCHSET 0/5] xfsprogs: random fixes for 6.2

[PATCHSET 0/5] xfsprogs: random fixes for 6.2

[Darrick J. Wong]

Hi all,

This is a rollup of all the random fixes I've collected for xfsprogs 6.2.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=xfsprogs-fixes-6.2
---
 io/bmap.c          |    2 +-
 io/open.c          |    3 ++-
 mkfs/proto.c       |   23 ++++++++++++++++++++++-
 scrub/fscounters.c |    2 +-
 spaceman/freesp.c  |    1 -
 5 files changed, 26 insertions(+), 5 deletions(-)

[PATCH 1/5] xfs_spaceman: fix broken -g behavior in freesp command

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Don't zero out the histogram bucket count when turning on group summary
mode -- this will screw up the data structures and it's pointless.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 spaceman/freesp.c |    1 -
 1 file changed, 1 deletion(-)


diff --git a/spaceman/freesp.c b/spaceman/freesp.c
index 423568a4248..70dcdb5c923 100644
--- a/spaceman/freesp.c
+++ b/spaceman/freesp.c
@@ -284,7 +284,6 @@ init(
 			speced = 1;
 			break;
 		case 'g':
-			histcount = 0;
 			gflag++;
 			break;
 		case 'h':

[PATCH 2/5] xfs_scrub: fix broken realtime free blocks unit conversions

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

r_blocks is in units of fs blocks, but freertx is in units of realtime
extents.  Add the missing conversion factor so we don't end up with
bogus things like this:

Pretend that sda and sdb are both 100T volumes.

# mkfs.xfs -f /dev/sda -b -r rtdev=/dev/sdb,extsize=2m
# mount /dev/sda /mnt -o rtdev=/dev/sdb
# xfs_scrub -dTvn /mnt
<snip>
Phase 7: Check summary counters.
3.5TiB data used;  99.8TiB realtime data used;  55 inodes used.
2.0GiB data found; 50.0MiB realtime data found; 55 inodes found.
55 inodes counted; 0 inodes checked.

We just created the filesystem, the realtime volume should be empty.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 scrub/fscounters.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/scrub/fscounters.c b/scrub/fscounters.c
index f21b24e0935..3ceae3715dc 100644
--- a/scrub/fscounters.c
+++ b/scrub/fscounters.c
@@ -138,7 +138,7 @@ scrub_scan_estimate_blocks(
 	*d_blocks = ctx->mnt.fsgeom.datablocks;
 	*d_bfree = fc.freedata;
 	*r_blocks = ctx->mnt.fsgeom.rtblocks;
-	*r_bfree = fc.freertx;
+	*r_bfree = fc.freertx * ctx->mnt.fsgeom.rtextsize;
 	*f_files_used = fc.allocino - fc.freeino;
 
 	return 0;

[PATCH 3/5] xfs_io: set fs_path when opening files on foreign filesystems

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Ted noticed that the following command:

$ xfs_io -c 'fsmap -d 0 0' /mnt
xfs_io: xfsctl(XFS_IOC_GETFSMAP) iflags=0x0 ["/mnt"]: Invalid argument

doesn't work on an ext4 filesystem.  The above command is supposed to
issue a GETFSMAP query against the "data" device.  Although the manpage
doesn't claim support for ext4, it turns out that this you get this
trace data:

          xfs_io-4144  [002]   210.965642: ext4_getfsmap_low_key: dev
7:0 keydev 163:2567 block 0 len 0 owner 0 flags 0x0
          xfs_io-4144  [002]   210.965645: ext4_getfsmap_high_key: dev
7:0 keydev 32:5277:0 block 0 len 0 owner -1 flags 0xffffffff

Notice the random garbage in the keydev field -- this happens because
openfile (in xfs_io) doesn't initialize *fs_path if the caller doesn't
supply a geometry structure or the opened file isn't on an XFS
filesystem.  IOWs, we feed random heap garbage to the kernel, and the
kernel rejects the call unnecessarily.

Fix this to set the fspath information even for foreign filesystems.

Reported-by: tytso@mit.edu
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 io/open.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


diff --git a/io/open.c b/io/open.c
index d8072664c16..15850b5557b 100644
--- a/io/open.c
+++ b/io/open.c
@@ -116,7 +116,7 @@ openfile(
 	}
 
 	if (!geom || !platform_test_xfs_fd(fd))
-		return fd;
+		goto set_fspath;
 
 	if (flags & IO_PATH) {
 		/* Can't call ioctl() on O_PATH fds */
@@ -150,6 +150,7 @@ openfile(
 		}
 	}
 
+set_fspath:
 	if (fs_path) {
 		fsp = fs_table_lookup(path, FS_MOUNT_POINT);
 		if (!fsp)

[PATCH 4/5] xfs_io: fix bmap command not detecting realtime files with xattrs

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Fix the bmap command so that it will detect a realtime file if any of
the other file flags (e.g. xattrs) are set.  Observed via xfs/556.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 io/bmap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/io/bmap.c b/io/bmap.c
index 27383ca6037..722a389baaa 100644
--- a/io/bmap.c
+++ b/io/bmap.c
@@ -118,7 +118,7 @@ bmap_f(
 			return 0;
 		}
 
-		if (fsx.fsx_xflags == FS_XFLAG_REALTIME) {
+		if (fsx.fsx_xflags & FS_XFLAG_REALTIME) {
 			/*
 			 * ag info not applicable to rt, continue
 			 * without ag output.

[PATCH 5/5] mkfs: substitute slashes with spaces in protofiles

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

A user requested the ability to specify directory entry names in a
protofile that have spaces in them.  The protofile format itself does
not allow spaces (yay 1973-era protofiles!) but it does allow slashes.
Slashes aren't allowed in directory entry names, so we'll permit this
one gross hack.

/
0 0
d--775 1000 1000
: Descending path /code/t/fstests
 get/isk.sh   ---775 1000 1000 /code/t/fstests/getdisk.sh
$

Will produce "get isk.h" in the root directory.

Requested-by: Daan De Meyer <daan.j.demeyer@gmail.com>
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 mkfs/proto.c |   23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)


diff --git a/mkfs/proto.c b/mkfs/proto.c
index 68ecdbf3632..bf8de0189db 100644
--- a/mkfs/proto.c
+++ b/mkfs/proto.c
@@ -171,6 +171,27 @@ getstr(
 	return NULL;
 }
 
+/* Extract directory entry name from a protofile. */
+static char *
+getdirentname(
+	char	**pp)
+{
+	char	*p = getstr(pp);
+	char	*c = p;
+
+	if (!p)
+		return NULL;
+
+	/* Replace slash with space because slashes aren't allowed. */
+	while (*c) {
+		if (*c == '/')
+			*c = ' ';
+		c++;
+	}
+
+	return p;
+}
+
 static void
 rsvfile(
 	xfs_mount_t	*mp,
@@ -580,7 +601,7 @@ parseproto(
 			rtinit(mp);
 		tp = NULL;
 		for (;;) {
-			name = getstr(pp);
+			name = getdirentname(pp);
 			if (!name)
 				break;
 			if (strcmp(name, "$") == 0)

Re: [PATCH 1/5] xfs_spaceman: fix broken -g behavior in freesp command

[Carlos Maiolino]

On Thu, Feb 16, 2023 at 01:52:53PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Don't zero out the histogram bucket count when turning on group summary
> mode -- this will screw up the data structures and it's pointless.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> ---
>  spaceman/freesp.c |    1 -
>  1 file changed, 1 deletion(-)
> 
> 
> diff --git a/spaceman/freesp.c b/spaceman/freesp.c
> index 423568a4248..70dcdb5c923 100644
> --- a/spaceman/freesp.c
> +++ b/spaceman/freesp.c
> @@ -284,7 +284,6 @@ init(
>  			speced = 1;
>  			break;
>  		case 'g':
> -			histcount = 0;
>  			gflag++;
>  			break;
>  		case 'h':
> 

-- 
Carlos Maiolino

Re: [PATCH 2/5] xfs_scrub: fix broken realtime free blocks unit conversions

[Carlos Maiolino]

On Thu, Feb 16, 2023 at 01:52:58PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> r_blocks is in units of fs blocks, but freertx is in units of realtime
> extents.  Add the missing conversion factor so we don't end up with
> bogus things like this:
> 
> Pretend that sda and sdb are both 100T volumes.
> 
> # mkfs.xfs -f /dev/sda -b -r rtdev=/dev/sdb,extsize=2m
> # mount /dev/sda /mnt -o rtdev=/dev/sdb
> # xfs_scrub -dTvn /mnt
> <snip>
> Phase 7: Check summary counters.
> 3.5TiB data used;  99.8TiB realtime data used;  55 inodes used.
> 2.0GiB data found; 50.0MiB realtime data found; 55 inodes found.
> 55 inodes counted; 0 inodes checked.
> 
> We just created the filesystem, the realtime volume should be empty.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> ---
>  scrub/fscounters.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/scrub/fscounters.c b/scrub/fscounters.c
> index f21b24e0935..3ceae3715dc 100644
> --- a/scrub/fscounters.c
> +++ b/scrub/fscounters.c
> @@ -138,7 +138,7 @@ scrub_scan_estimate_blocks(
>  	*d_blocks = ctx->mnt.fsgeom.datablocks;
>  	*d_bfree = fc.freedata;
>  	*r_blocks = ctx->mnt.fsgeom.rtblocks;
> -	*r_bfree = fc.freertx;
> +	*r_bfree = fc.freertx * ctx->mnt.fsgeom.rtextsize;
>  	*f_files_used = fc.allocino - fc.freeino;
> 
>  	return 0;
> 

-- 
Carlos Maiolino

Re: [PATCH 3/5] xfs_io: set fs_path when opening files on foreign filesystems

[Carlos Maiolino]

On Thu, Feb 16, 2023 at 01:53:04PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Ted noticed that the following command:
> 
> $ xfs_io -c 'fsmap -d 0 0' /mnt
> xfs_io: xfsctl(XFS_IOC_GETFSMAP) iflags=0x0 ["/mnt"]: Invalid argument
> 
> doesn't work on an ext4 filesystem.  The above command is supposed to
> issue a GETFSMAP query against the "data" device.  Although the manpage
> doesn't claim support for ext4, it turns out that this you get this
> trace data:
> 
>           xfs_io-4144  [002]   210.965642: ext4_getfsmap_low_key: dev
> 7:0 keydev 163:2567 block 0 len 0 owner 0 flags 0x0
>           xfs_io-4144  [002]   210.965645: ext4_getfsmap_high_key: dev
> 7:0 keydev 32:5277:0 block 0 len 0 owner -1 flags 0xffffffff
> 
> Notice the random garbage in the keydev field -- this happens because
> openfile (in xfs_io) doesn't initialize *fs_path if the caller doesn't
> supply a geometry structure or the opened file isn't on an XFS
> filesystem.  IOWs, we feed random heap garbage to the kernel, and the
> kernel rejects the call unnecessarily.
> 
> Fix this to set the fspath information even for foreign filesystems.
> 
> Reported-by: tytso@mit.edu
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> ---
>  io/open.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/io/open.c b/io/open.c
> index d8072664c16..15850b5557b 100644
> --- a/io/open.c
> +++ b/io/open.c
> @@ -116,7 +116,7 @@ openfile(
>  	}
> 
>  	if (!geom || !platform_test_xfs_fd(fd))
> -		return fd;
> +		goto set_fspath;
> 
>  	if (flags & IO_PATH) {
>  		/* Can't call ioctl() on O_PATH fds */
> @@ -150,6 +150,7 @@ openfile(
>  		}
>  	}
> 
> +set_fspath:
>  	if (fs_path) {
>  		fsp = fs_table_lookup(path, FS_MOUNT_POINT);
>  		if (!fsp)
> 

-- 
Carlos Maiolino

Re: [PATCH 4/5] xfs_io: fix bmap command not detecting realtime files with xattrs

[Carlos Maiolino]

On Thu, Feb 16, 2023 at 01:53:10PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Fix the bmap command so that it will detect a realtime file if any of
> the other file flags (e.g. xattrs) are set.  Observed via xfs/556.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> ---
>  io/bmap.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/io/bmap.c b/io/bmap.c
> index 27383ca6037..722a389baaa 100644
> --- a/io/bmap.c
> +++ b/io/bmap.c
> @@ -118,7 +118,7 @@ bmap_f(
>  			return 0;
>  		}
> 
> -		if (fsx.fsx_xflags == FS_XFLAG_REALTIME) {
> +		if (fsx.fsx_xflags & FS_XFLAG_REALTIME) {
>  			/*
>  			 * ag info not applicable to rt, continue
>  			 * without ag output.
> 

-- 
Carlos Maiolino

Re: [PATCH 5/5] mkfs: substitute slashes with spaces in protofiles

[Carlos Maiolino]

On Thu, Feb 16, 2023 at 01:53:15PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> A user requested the ability to specify directory entry names in a
> protofile that have spaces in them.  The protofile format itself does
> not allow spaces (yay 1973-era protofiles!) but it does allow slashes.
> Slashes aren't allowed in directory entry names, so we'll permit this
> one gross hack.
> 
> /
> 0 0
> d--775 1000 1000
> : Descending path /code/t/fstests
>  get/isk.sh   ---775 1000 1000 /code/t/fstests/getdisk.sh
> $
> 
> Will produce "get isk.h" in the root directory.
> 

While I don't really mind this patch, it seems strange to me to simply replace a
slash with a space in lieu of failing the prototype with an 'invalid character'
error message, or something like that.
With this patch, an user could be mistakenly assuming the get/isk.sh path will
be created, and instead, what's gonna be created is a file with a space.
I don't really mind it, but I think we could be misleading users.


> Requested-by: Daan De Meyer <daan.j.demeyer@gmail.com>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  mkfs/proto.c |   23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/mkfs/proto.c b/mkfs/proto.c
> index 68ecdbf3632..bf8de0189db 100644
> --- a/mkfs/proto.c
> +++ b/mkfs/proto.c
> @@ -171,6 +171,27 @@ getstr(
>  	return NULL;
>  }
> 
> +/* Extract directory entry name from a protofile. */
> +static char *
> +getdirentname(
> +	char	**pp)
> +{
> +	char	*p = getstr(pp);
> +	char	*c = p;
> +
> +	if (!p)
> +		return NULL;
> +
> +	/* Replace slash with space because slashes aren't allowed. */
> +	while (*c) {
> +		if (*c == '/')
> +			*c = ' ';
> +		c++;
> +	}
> +
> +	return p;
> +}
> +
>  static void
>  rsvfile(
>  	xfs_mount_t	*mp,
> @@ -580,7 +601,7 @@ parseproto(
>  			rtinit(mp);
>  		tp = NULL;
>  		for (;;) {
> -			name = getstr(pp);
> +			name = getdirentname(pp);
>  			if (!name)
>  				break;
>  			if (strcmp(name, "$") == 0)
> 

-- 
Carlos Maiolino

Re: [PATCH 5/5] mkfs: substitute slashes with spaces in protofiles

[Darrick J. Wong]

On Wed, Feb 22, 2023 at 10:03:03AM +0100, Carlos Maiolino wrote:
> On Thu, Feb 16, 2023 at 01:53:15PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > A user requested the ability to specify directory entry names in a
> > protofile that have spaces in them.  The protofile format itself does
> > not allow spaces (yay 1973-era protofiles!) but it does allow slashes.
> > Slashes aren't allowed in directory entry names, so we'll permit this
> > one gross hack.
> > 
> > /
> > 0 0
> > d--775 1000 1000
> > : Descending path /code/t/fstests
> >  get/isk.sh   ---775 1000 1000 /code/t/fstests/getdisk.sh
> > $
> > 
> > Will produce "get isk.h" in the root directory.
> > 
> 
> While I don't really mind this patch, it seems strange to me to simply replace a
> slash with a space in lieu of failing the prototype with an 'invalid character'
> error message, or something like that.
> With this patch, an user could be mistakenly assuming the get/isk.sh path will
> be created, and instead, what's gonna be created is a file with a space.
> I don't really mind it, but I think we could be misleading users.

Hmm.  I agree that it /does/ look weird.  I guess we could invent
suboptions for mkfs -p:

   mkfs.xfs -p slashes_are_spaces=1,/tmp/protofile

Which would turn on this odd looking functionalty?  How does that sound?

Sidebar:

As it is now, the slash gets passed to _addname, with the result that
mkfs will error out.

Another thing I (just) remembered is that the proto.c will also pick up
nulls and pass them to addname, so:

   get\000isk.sh ---775 1000 1000 /code/t/fstests/getdisk.sh

will also cause mkfs to fail the format.  I suppose that will also
require patching...

--D

> 
> > Requested-by: Daan De Meyer <daan.j.demeyer@gmail.com>
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  mkfs/proto.c |   23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> > 
> > 
> > diff --git a/mkfs/proto.c b/mkfs/proto.c
> > index 68ecdbf3632..bf8de0189db 100644
> > --- a/mkfs/proto.c
> > +++ b/mkfs/proto.c
> > @@ -171,6 +171,27 @@ getstr(
> >  	return NULL;
> >  }
> > 
> > +/* Extract directory entry name from a protofile. */
> > +static char *
> > +getdirentname(
> > +	char	**pp)
> > +{
> > +	char	*p = getstr(pp);
> > +	char	*c = p;
> > +
> > +	if (!p)
> > +		return NULL;
> > +
> > +	/* Replace slash with space because slashes aren't allowed. */
> > +	while (*c) {
> > +		if (*c == '/')
> > +			*c = ' ';
> > +		c++;
> > +	}
> > +
> > +	return p;
> > +}
> > +
> >  static void
> >  rsvfile(
> >  	xfs_mount_t	*mp,
> > @@ -580,7 +601,7 @@ parseproto(
> >  			rtinit(mp);
> >  		tp = NULL;
> >  		for (;;) {
> > -			name = getstr(pp);
> > +			name = getdirentname(pp);
> >  			if (!name)
> >  				break;
> >  			if (strcmp(name, "$") == 0)
> > 
> 
> -- 
> Carlos Maiolino

Re: [PATCH 5/5] mkfs: substitute slashes with spaces in protofiles

[Carlos Maiolino]

On Wed, Feb 22, 2023 at 08:37:03AM -0800, Darrick J. Wong wrote:
> On Wed, Feb 22, 2023 at 10:03:03AM +0100, Carlos Maiolino wrote:
> > On Thu, Feb 16, 2023 at 01:53:15PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > >
> > > A user requested the ability to specify directory entry names in a
> > > protofile that have spaces in them.  The protofile format itself does
> > > not allow spaces (yay 1973-era protofiles!) but it does allow slashes.
> > > Slashes aren't allowed in directory entry names, so we'll permit this
> > > one gross hack.
> > >
> > > /
> > > 0 0
> > > d--775 1000 1000
> > > : Descending path /code/t/fstests
> > >  get/isk.sh   ---775 1000 1000 /code/t/fstests/getdisk.sh
> > > $
> > >
> > > Will produce "get isk.h" in the root directory.
> > >
> >
> > While I don't really mind this patch, it seems strange to me to simply replace a
> > slash with a space in lieu of failing the prototype with an 'invalid character'
> > error message, or something like that.
> > With this patch, an user could be mistakenly assuming the get/isk.sh path will
> > be created, and instead, what's gonna be created is a file with a space.
> > I don't really mind it, but I think we could be misleading users.
> 
> Hmm.  I agree that it /does/ look weird.  I guess we could invent
> suboptions for mkfs -p:

Sorry, I don't mean to give you more trouble, but silently replacing a character
by another one does indeed looks weird to me.

> 
>    mkfs.xfs -p slashes_are_spaces=1,/tmp/protofile


> 
> Which would turn on this odd looking functionalty?  How does that sound?

this sounds better to me :)

> 
> Sidebar:
> 
> As it is now, the slash gets passed to _addname, with the result that
> mkfs will error out.
> 
> Another thing I (just) remembered is that the proto.c will also pick up
> nulls and pass them to addname, so:
> 
>    get\000isk.sh ---775 1000 1000 /code/t/fstests/getdisk.sh
> 
> will also cause mkfs to fail the format.  I suppose that will also
> require patching...

Interesting.
/me never played much with protofiles

I'll pickup the first 4 patches of this series tomorrow and skip this one for
now. I plan to release a 6.2 soon, as we already have linux 6.2 release, but I
want to do a libxfs sync first, hopefully I'll have the bandwidth tomorrow and
next week release 6.2. will see :)


> 
> >
> > > Requested-by: Daan De Meyer <daan.j.demeyer@gmail.com>
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  mkfs/proto.c |   23 ++++++++++++++++++++++-
> > >  1 file changed, 22 insertions(+), 1 deletion(-)
> > >
> > >
> > > diff --git a/mkfs/proto.c b/mkfs/proto.c
> > > index 68ecdbf3632..bf8de0189db 100644
> > > --- a/mkfs/proto.c
> > > +++ b/mkfs/proto.c
> > > @@ -171,6 +171,27 @@ getstr(
> > >  	return NULL;
> > >  }
> > >
> > > +/* Extract directory entry name from a protofile. */
> > > +static char *
> > > +getdirentname(
> > > +	char	**pp)
> > > +{
> > > +	char	*p = getstr(pp);
> > > +	char	*c = p;
> > > +
> > > +	if (!p)
> > > +		return NULL;
> > > +
> > > +	/* Replace slash with space because slashes aren't allowed. */
> > > +	while (*c) {
> > > +		if (*c == '/')
> > > +			*c = ' ';
> > > +		c++;
> > > +	}
> > > +
> > > +	return p;
> > > +}
> > > +
> > >  static void
> > >  rsvfile(
> > >  	xfs_mount_t	*mp,
> > > @@ -580,7 +601,7 @@ parseproto(
> > >  			rtinit(mp);
> > >  		tp = NULL;
> > >  		for (;;) {
> > > -			name = getstr(pp);
> > > +			name = getdirentname(pp);
> > >  			if (!name)
> > >  				break;
> > >  			if (strcmp(name, "$") == 0)
> > >
> >
> > --
> > Carlos Maiolino

-- 
Carlos Maiolino

Re: [PATCH 5/5] mkfs: substitute slashes with spaces in protofiles

[Darrick J. Wong]

On Thu, Feb 23, 2023 at 09:48:39AM +0100, Carlos Maiolino wrote:
> On Wed, Feb 22, 2023 at 08:37:03AM -0800, Darrick J. Wong wrote:
> > On Wed, Feb 22, 2023 at 10:03:03AM +0100, Carlos Maiolino wrote:
> > > On Thu, Feb 16, 2023 at 01:53:15PM -0800, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > >
> > > > A user requested the ability to specify directory entry names in a
> > > > protofile that have spaces in them.  The protofile format itself does
> > > > not allow spaces (yay 1973-era protofiles!) but it does allow slashes.
> > > > Slashes aren't allowed in directory entry names, so we'll permit this
> > > > one gross hack.
> > > >
> > > > /
> > > > 0 0
> > > > d--775 1000 1000
> > > > : Descending path /code/t/fstests
> > > >  get/isk.sh   ---775 1000 1000 /code/t/fstests/getdisk.sh
> > > > $
> > > >
> > > > Will produce "get isk.h" in the root directory.
> > > >
> > >
> > > While I don't really mind this patch, it seems strange to me to simply replace a
> > > slash with a space in lieu of failing the prototype with an 'invalid character'
> > > error message, or something like that.
> > > With this patch, an user could be mistakenly assuming the get/isk.sh path will
> > > be created, and instead, what's gonna be created is a file with a space.
> > > I don't really mind it, but I think we could be misleading users.
> > 
> > Hmm.  I agree that it /does/ look weird.  I guess we could invent
> > suboptions for mkfs -p:
> 
> Sorry, I don't mean to give you more trouble, but silently replacing a character
> by another one does indeed looks weird to me.

Agreed.

> > 
> >    mkfs.xfs -p slashes_are_spaces=1,/tmp/protofile
> 
> 
> > 
> > Which would turn on this odd looking functionalty?  How does that sound?
> 
> this sounds better to me :)

Ok, will do.

> > 
> > Sidebar:
> > 
> > As it is now, the slash gets passed to _addname, with the result that
> > mkfs will error out.
> > 
> > Another thing I (just) remembered is that the proto.c will also pick up
> > nulls and pass them to addname, so:
> > 
> >    get\000isk.sh ---775 1000 1000 /code/t/fstests/getdisk.sh
> > 
> > will also cause mkfs to fail the format.  I suppose that will also
> > require patching...
> 
> Interesting.
> /me never played much with protofiles
> 
> I'll pickup the first 4 patches of this series tomorrow and skip this one for
> now. I plan to release a 6.2 soon, as we already have linux 6.2 release, but I
> want to do a libxfs sync first, hopefully I'll have the bandwidth tomorrow and
> next week release 6.2. will see :)

Ok.  The slashes_are_spaces=1 patches are pretty trivial, so I'll send
them in Tuesday's batch.

--D

> 
> > 
> > >
> > > > Requested-by: Daan De Meyer <daan.j.demeyer@gmail.com>
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > >  mkfs/proto.c |   23 ++++++++++++++++++++++-
> > > >  1 file changed, 22 insertions(+), 1 deletion(-)
> > > >
> > > >
> > > > diff --git a/mkfs/proto.c b/mkfs/proto.c
> > > > index 68ecdbf3632..bf8de0189db 100644
> > > > --- a/mkfs/proto.c
> > > > +++ b/mkfs/proto.c
> > > > @@ -171,6 +171,27 @@ getstr(
> > > >  	return NULL;
> > > >  }
> > > >
> > > > +/* Extract directory entry name from a protofile. */
> > > > +static char *
> > > > +getdirentname(
> > > > +	char	**pp)
> > > > +{
> > > > +	char	*p = getstr(pp);
> > > > +	char	*c = p;
> > > > +
> > > > +	if (!p)
> > > > +		return NULL;
> > > > +
> > > > +	/* Replace slash with space because slashes aren't allowed. */
> > > > +	while (*c) {
> > > > +		if (*c == '/')
> > > > +			*c = ' ';
> > > > +		c++;
> > > > +	}
> > > > +
> > > > +	return p;
> > > > +}
> > > > +
> > > >  static void
> > > >  rsvfile(
> > > >  	xfs_mount_t	*mp,
> > > > @@ -580,7 +601,7 @@ parseproto(
> > > >  			rtinit(mp);
> > > >  		tp = NULL;
> > > >  		for (;;) {
> > > > -			name = getstr(pp);
> > > > +			name = getdirentname(pp);
> > > >  			if (!name)
> > > >  				break;
> > > >  			if (strcmp(name, "$") == 0)
> > > >
> > >
> > > --
> > > Carlos Maiolino
> 
> -- 
> Carlos Maiolino