Re: [PATCH 08/10] xfs_spaceman: Free space mapping command

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Jun 14, 2017 at 11:04:11AM -0500, Eric Sandeen wrote:
> 
> On 6/2/17 2:52 PM, Darrick J. Wong wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Add freespace mapping tool modelled on the xfs_db freesp command.
> > The advantage of this command over xfs_db is that it can be done
> > online and is coherent with concurrent modifications to the
> > filesystem.
> > 
> > This requires the kernel to support the XFS_IOC_GETFSMAP ioctl to map
> > free space indexes.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > [darrick: port from FIEMAPFS to GETFSMAP]
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> 
> > +static int
> > +init(
> > +	int		argc,
> > +	char		**argv)
> > +{
> > +	int		c;
> > +	int		speced = 0;
> > +
> > +	agcount = dumpflag = equalsize = multsize = optind = 0;
> > +	histcount = seen1 = summaryflag = 0;
> 
> ew, setting histcount to 0 (already done, right?  it's global?)
> and checking it here but never setting it is a little confusing.

Yes, I'll get rid of the redundant initializers.

> Maybe the comment below would help.

Ok.

> > +	totblocks = totexts = 0;
> > +	aglist = NULL;
> > +	hist = NULL;
> > +	rtflag = false;
> > +	while ((c = getopt(argc, argv, "a:bde:h:m:rs")) != EOF) {
> > +		switch (c) {
> > +		case 'a':
> > +			aglistadd(optarg);
> > +			break;
> > +		case 'b':
> > +			if (speced)
> > +				return 0;
> > +			multsize = 2;
> > +			speced = 1;
> > +			break;
> > +		case 'd':
> > +			dumpflag = 1;
> > +			break;
> > +		case 'e':
> > +			if (speced)
> > +				return 0;
> > +			equalsize = atoi(optarg);
> > +			speced = 1;
> > +			break;
> > +		case 'h':
> > +			if (speced && !histcount)
> > +				return 0;
> 
> maybe:			/* increments histcount */

Fixed.

> > +			addhistent(atoi(optarg));
> > +			speced = 1;
> > +			break;
> > +		case 'm':
> > +			if (speced)
> > +				return 0;
> > +			multsize = atoi(optarg);
> > +			speced = 1;
> > +			break;
> > +		case 'r':
> > +			rtflag = true;
> > +			break;
> > +		case 's':
> > +			summaryflag = 1;
> > +			break;
> > +		case '?':
> > +			return 0;
> > +		}
> > +	}
> > +	if (optind != argc)
> > +		return 0;
> > +	if (!speced)
> > +		multsize = 2;
> 
> Manpage indicates that -b, -e, and -h are mutually exclusive, but that's
> not enforced here (?)
> 
> Specifying more than one seems to break the command:
> 
> xfs_spaceman> freesp -b
>    from      to extents  blocks    pct
>       2       3       1       3   0.00
>   32768   65536       4  259537 100.00
> xfs_spaceman> freesp -e 4096
>    from      to extents  blocks    pct
>       1    4096       1       3   0.00
>   61441   65536       4  259537 100.00
> xfs_spaceman> freesp -e 4096 -b
> xfs_spaceman> 
> 
> Oh I see, if more than one is "speced" init returns 0 (silently).
> 
> I'd document what speced is for, and I guess print a warning about
> the exclusive nature of the 3 arguments in each case if re-spec'd
> before returning.

I made a note of that in the help screen and redirected all the overspec
cases to print the help screen instead of silently returning.

> And note the exclusive nature somehow in the help.
> (hm, looks like -m is in this exclusive group too?)
> 
> in gneneral need to sanitize behavior w/ built in help as well
> as manpage...

Ok.

--D

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html