[PATCHSET 0/2] xfs_io: small fixes to funshare command

[PATCHSET 0/2] xfs_io: small fixes to funshare command

[Darrick J. Wong]

Hi all,

A couple of small fixes to the funshare command.

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=funshare-fixes
---
 io/prealloc.c |   33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

[PATCH 1/2] xfs_io: fix broken funshare_cmd usage

[Darrick J. Wong]

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

Create a funshare_cmd and use that to store information about the
xfs_io funshare command instead of overwriting the contents of
fzero_cmd.  This fixes confusing output like:

$ xfs_io -c 'fzero 2 3 --help' /
fzero: invalid option -- '-'
funshare off len -- unshares shared blocks within the range

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 io/prealloc.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)


diff --git a/io/prealloc.c b/io/prealloc.c
index 382e8119..2ae8afe9 100644
--- a/io/prealloc.c
+++ b/io/prealloc.c
@@ -43,6 +43,7 @@ static cmdinfo_t fpunch_cmd;
 static cmdinfo_t fcollapse_cmd;
 static cmdinfo_t finsert_cmd;
 static cmdinfo_t fzero_cmd;
+static cmdinfo_t funshare_cmd;
 #endif
 
 static int
@@ -467,14 +468,14 @@ prealloc_init(void)
 	_("zeroes space and eliminates holes by preallocating");
 	add_command(&fzero_cmd);
 
-	fzero_cmd.name = "funshare";
-	fzero_cmd.cfunc = funshare_f;
-	fzero_cmd.argmin = 2;
-	fzero_cmd.argmax = 2;
-	fzero_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
-	fzero_cmd.args = _("off len");
-	fzero_cmd.oneline =
+	funshare_cmd.name = "funshare";
+	funshare_cmd.cfunc = funshare_f;
+	funshare_cmd.argmin = 2;
+	funshare_cmd.argmax = 2;
+	funshare_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
+	funshare_cmd.args = _("off len");
+	funshare_cmd.oneline =
 	_("unshares shared blocks within the range");
-	add_command(&fzero_cmd);
+	add_command(&funshare_cmd);
 #endif	/* HAVE_FALLOCATE */
 }

[PATCH 2/2] xfs_io: clean up the funshare command a bit

[Darrick J. Wong]

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

Add proper argument parsing to the funshare command so that when you
pass it nonexistent --help it will print the help instead of complaining
that it can't convert that to a number.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 io/prealloc.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)


diff --git a/io/prealloc.c b/io/prealloc.c
index 2ae8afe9..94cf326f 100644
--- a/io/prealloc.c
+++ b/io/prealloc.c
@@ -346,16 +346,24 @@ funshare_f(
 	char		**argv)
 {
 	xfs_flock64_t	segment;
+	int		c;
 	int		mode = FALLOC_FL_UNSHARE_RANGE;
-	int		index = 1;
 
-	if (!offset_length(argv[index], argv[index + 1], &segment)) {
+	while ((c = getopt(argc, argv, "")) != EOF) {
+		switch (c) {
+		default:
+			command_usage(&funshare_cmd);
+		}
+	}
+        if (optind != argc - 2)
+                return command_usage(&funshare_cmd);
+
+	if (!offset_length(argv[optind], argv[optind + 1], &segment)) {
 		exitcode = 1;
 		return 0;
 	}
 
-	if (fallocate(file->fd, mode,
-			segment.l_start, segment.l_len)) {
+	if (fallocate(file->fd, mode, segment.l_start, segment.l_len)) {
 		perror("fallocate");
 		exitcode = 1;
 		return 0;

Re: [PATCH 1/2] xfs_io: fix broken funshare_cmd usage

[Christoph Hellwig]

On Fri, Jul 02, 2021 at 07:58:02PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Create a funshare_cmd and use that to store information about the
> xfs_io funshare command instead of overwriting the contents of
> fzero_cmd.  This fixes confusing output like:
> 
> $ xfs_io -c 'fzero 2 3 --help' /
> fzero: invalid option -- '-'
> funshare off len -- unshares shared blocks within the range

Ooops, how did this manage to ever work?

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] xfs_io: clean up the funshare command a bit

[Christoph Hellwig]

On Fri, Jul 02, 2021 at 07:58:08PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Add proper argument parsing to the funshare command so that when you
> pass it nonexistent --help it will print the help instead of complaining
> that it can't convert that to a number.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  io/prealloc.c |   16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/io/prealloc.c b/io/prealloc.c
> index 2ae8afe9..94cf326f 100644
> --- a/io/prealloc.c
> +++ b/io/prealloc.c
> @@ -346,16 +346,24 @@ funshare_f(
>  	char		**argv)
>  {
>  	xfs_flock64_t	segment;
> +	int		c;
>  	int		mode = FALLOC_FL_UNSHARE_RANGE;
> -	int		index = 1;
>  
> -	if (!offset_length(argv[index], argv[index + 1], &segment)) {
> +	while ((c = getopt(argc, argv, "")) != EOF) {
> +		switch (c) {
> +		default:
> +			command_usage(&funshare_cmd);
> +		}

Do we really need this switch boilerplate?

> +	}
> +        if (optind != argc - 2)
> +                return command_usage(&funshare_cmd);

Spaces instead of tabs here.

Re: [PATCH 1/2] xfs_io: fix broken funshare_cmd usage

[Darrick J. Wong]

On Mon, Jul 05, 2021 at 04:16:42PM +0100, Christoph Hellwig wrote:
> On Fri, Jul 02, 2021 at 07:58:02PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Create a funshare_cmd and use that to store information about the
> > xfs_io funshare command instead of overwriting the contents of
> > fzero_cmd.  This fixes confusing output like:
> > 
> > $ xfs_io -c 'fzero 2 3 --help' /
> > fzero: invalid option -- '-'
> > funshare off len -- unshares shared blocks within the range
> 
> Ooops, how did this manage to ever work?

It "works" (in the sense that fzero and funshare issue the correct
fallocate modes) because add_command copies the contents of its struct
parameter into the internal command list.  The braindamage is limited to
any subsequent use of fzero_cmd, which (afaict) means the only way you'd
notice is through the help screens.

--D

> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] xfs_io: clean up the funshare command a bit

[Darrick J. Wong]

On Mon, Jul 05, 2021 at 04:20:06PM +0100, Christoph Hellwig wrote:
> On Fri, Jul 02, 2021 at 07:58:08PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Add proper argument parsing to the funshare command so that when you
> > pass it nonexistent --help it will print the help instead of complaining
> > that it can't convert that to a number.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  io/prealloc.c |   16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > 
> > diff --git a/io/prealloc.c b/io/prealloc.c
> > index 2ae8afe9..94cf326f 100644
> > --- a/io/prealloc.c
> > +++ b/io/prealloc.c
> > @@ -346,16 +346,24 @@ funshare_f(
> >  	char		**argv)
> >  {
> >  	xfs_flock64_t	segment;
> > +	int		c;
> >  	int		mode = FALLOC_FL_UNSHARE_RANGE;
> > -	int		index = 1;
> >  
> > -	if (!offset_length(argv[index], argv[index + 1], &segment)) {
> > +	while ((c = getopt(argc, argv, "")) != EOF) {
> > +		switch (c) {
> > +		default:
> > +			command_usage(&funshare_cmd);
> > +		}
> 
> Do we really need this switch boilerplate?

Sort of -- without it, you get interactions like:

$ xfs_io -c 'funshare 0 --help' /autoexec.bat
non-numeric length-argument -- --help

which is silly, since we could display the help screen.

The other solution might be to fix all the offset_length() callers to
emit command usage when the number parsing doesn't work, but that would
clutter up the error reporting when you try to feed it a number that
doesn't parse.

> > +	}
> > +        if (optind != argc - 2)
> > +                return command_usage(&funshare_cmd);
> 
> Spaces instead of tabs here.

Fixed.

--D

Re: [PATCH 1/2] xfs_io: fix broken funshare_cmd usage

[Carlos Maiolino]

On Fri, Jul 02, 2021 at 07:58:02PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Create a funshare_cmd and use that to store information about the
> xfs_io funshare command instead of overwriting the contents of
> fzero_cmd.  This fixes confusing output like:
> 
> $ xfs_io -c 'fzero 2 3 --help' /
> fzero: invalid option -- '-'
> funshare off len -- unshares shared blocks within the range

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


> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  io/prealloc.c |   17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> 
> diff --git a/io/prealloc.c b/io/prealloc.c
> index 382e8119..2ae8afe9 100644
> --- a/io/prealloc.c
> +++ b/io/prealloc.c
> @@ -43,6 +43,7 @@ static cmdinfo_t fpunch_cmd;
>  static cmdinfo_t fcollapse_cmd;
>  static cmdinfo_t finsert_cmd;
>  static cmdinfo_t fzero_cmd;
> +static cmdinfo_t funshare_cmd;
>  #endif
>  
>  static int
> @@ -467,14 +468,14 @@ prealloc_init(void)
>  	_("zeroes space and eliminates holes by preallocating");
>  	add_command(&fzero_cmd);
>  
> -	fzero_cmd.name = "funshare";
> -	fzero_cmd.cfunc = funshare_f;
> -	fzero_cmd.argmin = 2;
> -	fzero_cmd.argmax = 2;
> -	fzero_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
> -	fzero_cmd.args = _("off len");
> -	fzero_cmd.oneline =
> +	funshare_cmd.name = "funshare";
> +	funshare_cmd.cfunc = funshare_f;
> +	funshare_cmd.argmin = 2;
> +	funshare_cmd.argmax = 2;
> +	funshare_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
> +	funshare_cmd.args = _("off len");
> +	funshare_cmd.oneline =
>  	_("unshares shared blocks within the range");
> -	add_command(&fzero_cmd);
> +	add_command(&funshare_cmd);
>  #endif	/* HAVE_FALLOCATE */
>  }
> 

-- 
Carlos

Re: [PATCH 2/2] xfs_io: clean up the funshare command a bit

[Carlos Maiolino]

On Fri, Jul 02, 2021 at 07:58:08PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Add proper argument parsing to the funshare command so that when you
> pass it nonexistent --help it will print the help instead of complaining
> that it can't convert that to a number.

Looks ok to me despite the space/tab thing already mentioned by hch.

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

> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  io/prealloc.c |   16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/io/prealloc.c b/io/prealloc.c
> index 2ae8afe9..94cf326f 100644
> --- a/io/prealloc.c
> +++ b/io/prealloc.c
> @@ -346,16 +346,24 @@ funshare_f(
>  	char		**argv)
>  {
>  	xfs_flock64_t	segment;
> +	int		c;
>  	int		mode = FALLOC_FL_UNSHARE_RANGE;
> -	int		index = 1;
>  
> -	if (!offset_length(argv[index], argv[index + 1], &segment)) {
> +	while ((c = getopt(argc, argv, "")) != EOF) {
> +		switch (c) {
> +		default:
> +			command_usage(&funshare_cmd);
> +		}
> +	}
> +        if (optind != argc - 2)
> +                return command_usage(&funshare_cmd);
> +
> +	if (!offset_length(argv[optind], argv[optind + 1], &segment)) {
>  		exitcode = 1;
>  		return 0;
>  	}
>  
> -	if (fallocate(file->fd, mode,
> -			segment.l_start, segment.l_len)) {
> +	if (fallocate(file->fd, mode, segment.l_start, segment.l_len)) {
>  		perror("fallocate");
>  		exitcode = 1;
>  		return 0;
> 

-- 
Carlos

[PATCH 2/2] xfs_io: clean up the funshare command a bit

[Darrick J. Wong]

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

Add proper argument parsing to the funshare command so that when you
pass it nonexistent --help it will print the help instead of complaining
that it can't convert that to a number.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 io/prealloc.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)


diff --git a/io/prealloc.c b/io/prealloc.c
index 2ae8afe9..a8831c1b 100644
--- a/io/prealloc.c
+++ b/io/prealloc.c
@@ -346,16 +346,24 @@ funshare_f(
 	char		**argv)
 {
 	xfs_flock64_t	segment;
+	int		c;
 	int		mode = FALLOC_FL_UNSHARE_RANGE;
-	int		index = 1;
 
-	if (!offset_length(argv[index], argv[index + 1], &segment)) {
+	while ((c = getopt(argc, argv, "")) != EOF) {
+		switch (c) {
+		default:
+			command_usage(&funshare_cmd);
+		}
+	}
+	if (optind != argc - 2)
+		return command_usage(&funshare_cmd);
+
+	if (!offset_length(argv[optind], argv[optind + 1], &segment)) {
 		exitcode = 1;
 		return 0;
 	}
 
-	if (fallocate(file->fd, mode,
-			segment.l_start, segment.l_len)) {
+	if (fallocate(file->fd, mode, segment.l_start, segment.l_len)) {
 		perror("fallocate");
 		exitcode = 1;
 		return 0;