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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > [darrick: port from FIEMAPFS to GETFSMAP]
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
>
> > +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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
