| Message ID | 20190813051421.21137-2-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfsprogs: "Fix" --disable-static option | expand |
On Tue, Aug 13, 2019 at 03:14:19PM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Move the checks for zero block/sector size to the libfrog code > and return -1LL as an invalid value instead. Catch the invalid > value in mkfs and error out there instead of inside cvtnum. > > Also rename the libfrog block/sector size variables so they don't > shadow the mkfs global variables of the same name. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > libfrog/convert.c | 12 +++++--- > mkfs/xfs_mkfs.c | 71 ++++------------------------------------------- > 2 files changed, 14 insertions(+), 69 deletions(-) > > diff --git a/libfrog/convert.c b/libfrog/convert.c > index 8d4d4077b331..b5f3fc1238dd 100644 > --- a/libfrog/convert.c > +++ b/libfrog/convert.c > @@ -182,8 +182,8 @@ cvt_u16( > > long long > cvtnum( > - size_t blocksize, > - size_t sectorsize, > + size_t blksize, > + size_t sectsize, > char *s) > { > long long i; > @@ -202,9 +202,13 @@ cvtnum( > c = tolower(*sp); > switch (c) { > case 'b': > - return i * blocksize; > + if (!blksize) > + return -1LL; > + return i * blksize; > case 's': > - return i * sectorsize; > + if (!sectsize) > + return -1LL; > + return i * sectsize; > case 'k': > return KILOBYTES(i); > case 'm': > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > index 0adaa65d19f8..04063ca5b2c7 100644 > --- a/mkfs/xfs_mkfs.c > +++ b/mkfs/xfs_mkfs.c > @@ -942,69 +942,6 @@ unknown( > usage(); > } > > -long long > -cvtnum( > - unsigned int blksize, > - unsigned int sectsize, > - const char *s) > -{ > - long long i; > - char *sp; > - int c; > - > - i = strtoll(s, &sp, 0); > - if (i == 0 && sp == s) > - return -1LL; > - if (*sp == '\0') > - return i; > - > - if (sp[1] != '\0') > - return -1LL; > - > - if (*sp == 'b') { > - if (!blksize) { > - fprintf(stderr, > -_("Blocksize must be provided prior to using 'b' suffix.\n")); > - usage(); > - } else { > - return i * blksize; > - } > - } > - if (*sp == 's') { > - if (!sectsize) { > - fprintf(stderr, > -_("Sectorsize must be specified prior to using 's' suffix.\n")); Hmm, so this message is replaced with "Not a valid value or illegal suffix"? That's not anywhere near as helpful as the old message... maybe we should have this set errno or something so that callers can distinguish between "you sent garbled input" vs. "you need to set up blocksize /sectsize"... ? --D > - usage(); > - } else { > - return i * sectsize; > - } > - } > - > - c = tolower(*sp); > - switch (c) { > - case 'e': > - i *= 1024LL; > - /* fall through */ > - case 'p': > - i *= 1024LL; > - /* fall through */ > - case 't': > - i *= 1024LL; > - /* fall through */ > - case 'g': > - i *= 1024LL; > - /* fall through */ > - case 'm': > - i *= 1024LL; > - /* fall through */ > - case 'k': > - return i * 1024LL; > - default: > - break; > - } > - return -1LL; > -} > - > static void > check_device_type( > const char *name, > @@ -1347,9 +1284,13 @@ getnum( > * convert it ourselves to guarantee there is no trailing garbage in the > * number. > */ > - if (sp->convert) > + if (sp->convert) { > c = cvtnum(blocksize, sectorsize, str); > - else { > + if (c == -1LL) { > + illegal_option(str, opts, index, > + _("Not a valid value or illegal suffix")); > + } > + } else { > char *str_end; > > c = strtoll(str, &str_end, 0); > -- > 2.23.0.rc1 >
On Tue, Aug 13, 2019 at 07:24:14AM -0700, Darrick J. Wong wrote: > On Tue, Aug 13, 2019 at 03:14:19PM +1000, Dave Chinner wrote: > > From: Dave Chinner <dchinner@redhat.com> > > - } > > - if (*sp == 's') { > > - if (!sectsize) { > > - fprintf(stderr, > > -_("Sectorsize must be specified prior to using 's' suffix.\n")); > > Hmm, so this message is replaced with "Not a valid value or illegal suffix"? Actually, the error message is this: # mkfs.xfs -f -b size=1b /dev/vdc Invalid value 1b for -b size option. Not a valid value or illegal suffix It does actually tell you what the value is, what option is wrong, and the message shold be fairly clear that specifying the block size in using a "blocks" suffix is illegal. > That's not anywhere near as helpful as the old message... maybe we > should have this set errno or something so that callers can distinguish > between "you sent garbled input" vs. "you need to set up > blocksize /sectsize"... ? Actually, the error will only occur when you use -s size= or -b size= options, as if they are not specified we use the default values in mkfs and cvtnum is always called with a valid blocksize/sectorsize pair. i.e. This error only triggers when validating the base sector size/block size options because that occurs before we set the global varibles mkfs will use for cvtnum.... It's a chicken-egg thing, and I figured the error message prefix would be sufficient to point out the problem with the value suffic used for these kinda unusual corner cases. Cheers, Dave.
On Wed, Aug 14, 2019 at 07:29:36AM +1000, Dave Chinner wrote: > On Tue, Aug 13, 2019 at 07:24:14AM -0700, Darrick J. Wong wrote: > > On Tue, Aug 13, 2019 at 03:14:19PM +1000, Dave Chinner wrote: > > > From: Dave Chinner <dchinner@redhat.com> > > > - } > > > - if (*sp == 's') { > > > - if (!sectsize) { > > > - fprintf(stderr, > > > -_("Sectorsize must be specified prior to using 's' suffix.\n")); > > > > Hmm, so this message is replaced with "Not a valid value or illegal suffix"? > > Actually, the error message is this: > > # mkfs.xfs -f -b size=1b /dev/vdc > Invalid value 1b for -b size option. Not a valid value or illegal suffix > > It does actually tell you what the value is, what option is wrong, > and the message shold be fairly clear that specifying the block size > in using a "blocks" suffix is illegal. > > > That's not anywhere near as helpful as the old message... maybe we > > should have this set errno or something so that callers can distinguish > > between "you sent garbled input" vs. "you need to set up > > blocksize /sectsize"... ? > > Actually, the error will only occur when you use -s size= or -b > size= options, as if they are not specified we use the default > values in mkfs and cvtnum is always called with a valid > blocksize/sectorsize pair. i.e. This error only triggers when validating > the base sector size/block size options because that occurs before > we set the global varibles mkfs will use for cvtnum.... > > It's a chicken-egg thing, and I figured the error message prefix > would be sufficient to point out the problem with the value suffic > used for these kinda unusual corner cases. Heh, ok, carry on then. :) Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
diff --git a/libfrog/convert.c b/libfrog/convert.c index 8d4d4077b331..b5f3fc1238dd 100644 --- a/libfrog/convert.c +++ b/libfrog/convert.c @@ -182,8 +182,8 @@ cvt_u16( long long cvtnum( - size_t blocksize, - size_t sectorsize, + size_t blksize, + size_t sectsize, char *s) { long long i; @@ -202,9 +202,13 @@ cvtnum( c = tolower(*sp); switch (c) { case 'b': - return i * blocksize; + if (!blksize) + return -1LL; + return i * blksize; case 's': - return i * sectorsize; + if (!sectsize) + return -1LL; + return i * sectsize; case 'k': return KILOBYTES(i); case 'm': diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c index 0adaa65d19f8..04063ca5b2c7 100644 --- a/mkfs/xfs_mkfs.c +++ b/mkfs/xfs_mkfs.c @@ -942,69 +942,6 @@ unknown( usage(); } -long long -cvtnum( - unsigned int blksize, - unsigned int sectsize, - const char *s) -{ - long long i; - char *sp; - int c; - - i = strtoll(s, &sp, 0); - if (i == 0 && sp == s) - return -1LL; - if (*sp == '\0') - return i; - - if (sp[1] != '\0') - return -1LL; - - if (*sp == 'b') { - if (!blksize) { - fprintf(stderr, -_("Blocksize must be provided prior to using 'b' suffix.\n")); - usage(); - } else { - return i * blksize; - } - } - if (*sp == 's') { - if (!sectsize) { - fprintf(stderr, -_("Sectorsize must be specified prior to using 's' suffix.\n")); - usage(); - } else { - return i * sectsize; - } - } - - c = tolower(*sp); - switch (c) { - case 'e': - i *= 1024LL; - /* fall through */ - case 'p': - i *= 1024LL; - /* fall through */ - case 't': - i *= 1024LL; - /* fall through */ - case 'g': - i *= 1024LL; - /* fall through */ - case 'm': - i *= 1024LL; - /* fall through */ - case 'k': - return i * 1024LL; - default: - break; - } - return -1LL; -} - static void check_device_type( const char *name, @@ -1347,9 +1284,13 @@ getnum( * convert it ourselves to guarantee there is no trailing garbage in the * number. */ - if (sp->convert) + if (sp->convert) { c = cvtnum(blocksize, sectorsize, str); - else { + if (c == -1LL) { + illegal_option(str, opts, index, + _("Not a valid value or illegal suffix")); + } + } else { char *str_end; c = strtoll(str, &str_end, 0);