| Message ID | 20200803125018.16718-1-hsiangkao@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mkfs.xfs: introduce sunit/swidth validation helper | expand |
On Mon, Aug 03, 2020 at 08:50:18PM +0800, Gao Xiang wrote: > Currently stripe unit/width checking logic is all over xfsprogs. > So, refactor the same code snippet into a single validation helper > xfs_validate_stripe_factors(), including: > - integer overflows of either value > - sunit and swidth alignment wrt sector size > - if either sunit or swidth are zero, both should be zero > - swidth must be a multiple of sunit > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > --- > > This patch follows Darrick's original suggestion [1], yet I'm > not sure if I'm doing the right thing or if something is still > missing (e.g the meaning of six(ish) places)... So post it > right now... > > TBH, especially all these naming and the helper location (whether > in topology.c)...plus, click a dislike on calc_stripe_factors() > itself... > > (Hopefully hear some advice about this... Thanks!) > > [1] https://lore.kernel.org/r/20200515204802.GO6714@magnolia > > libfrog/topology.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++ > libfrog/topology.h | 15 ++++++++++++++ > mkfs/xfs_mkfs.c | 48 ++++++++++++++++++++++---------------------- > 3 files changed, 89 insertions(+), 24 deletions(-) > > diff --git a/libfrog/topology.c b/libfrog/topology.c > index b1b470c9..cf56fb03 100644 > --- a/libfrog/topology.c > +++ b/libfrog/topology.c > @@ -174,6 +174,41 @@ out: > return ret; > } > > +enum xfs_stripe_retcode > +xfs_validate_stripe_factors( libfrog functions (and enums) should be prefixed with libfrog, not xfs. LIBFROG_STRIPEVAL_{OK,SUNIT_MISALIGN, etc.} > + int sectorsize, > + int *sup, Errant space between "int" and "*sup". > + int *swp) Strange that a validator function has out parameters... Also, uh, .... full names, please. int *sunitp, int *swidthp) (I'm vaguely wondering why we use signed ints here vs. unsigned, but that isn't critical...) > +{ > + int sunit = *sup, swidth = *swp; > + > + if (sectorsize) { > + long long big_swidth; > + > + if (sunit % sectorsize) > + return XFS_STRIPE_RET_SUNIT_MISALIGN; > + > + sunit = (int)BTOBBT(sunit); Hmm. On input, *sup is in units of bytes, but on output it can be in units of 512b blocks? That is very surprising... > + big_swidth = (long long)sunit * swidth; > + > + if (big_swidth > INT_MAX) > + return XFS_STRIPE_RET_SWIDTH_OVERFLOW; > + swidth = big_swidth; > + } > + if ((sunit && !swidth) || (!sunit && swidth)) > + return XFS_STRIPE_RET_PARTIAL_VALID; > + > + if (sunit > swidth) > + return XFS_STRIPE_RET_SUNIT_TOO_LARGE; > + > + if (sunit && (swidth % sunit)) > + return XFS_STRIPE_RET_SWIDTH_MISALIGN; > + > + *sup = sunit; ...especially since in the !sectorsize case we don't change it at all. > + *swp = swidth; > + return XFS_STRIPE_RET_OK; > +} > + > static void blkid_get_topology( > const char *device, > int *sunit, > @@ -229,6 +264,21 @@ static void blkid_get_topology( > */ > *sunit = *sunit >> 9; > *swidth = *swidth >> 9; > + switch (xfs_validate_stripe_factors(0, sunit, swidth)) { > + case XFS_STRIPE_RET_OK: > + break; > + case XFS_STRIPE_RET_PARTIAL_VALID: > + fprintf(stderr, > +_("%s: Volume reports stripe unit of %d bytes and stripe width of %d bytes, ignoring.\n"), > + progname, BBTOB(*sunit), BBTOB(*swidth)); Needs a "/* fallthrough */" comment here. > + default: Why don't we warn about receiving garbage geometry that produces MISALIGN or OVERFLOW? > + /* > + * if firmware is broken, just give up and set both to zero, > + * we can't trust information from this device. > + */ > + *sunit = 0; > + *swidth = 0; > + } > > if (blkid_topology_get_alignment_offset(tp) != 0) { > fprintf(stderr, > diff --git a/libfrog/topology.h b/libfrog/topology.h > index 6fde868a..e8be26b2 100644 > --- a/libfrog/topology.h > +++ b/libfrog/topology.h > @@ -36,4 +36,19 @@ extern int > check_overwrite( > const char *device); > > +enum xfs_stripe_retcode { > + XFS_STRIPE_RET_OK = 0, > + XFS_STRIPE_RET_SUNIT_MISALIGN, > + XFS_STRIPE_RET_SWIDTH_OVERFLOW, > + XFS_STRIPE_RET_PARTIAL_VALID, > + XFS_STRIPE_RET_SUNIT_TOO_LARGE, > + XFS_STRIPE_RET_SWIDTH_MISALIGN, > +}; > + > +enum xfs_stripe_retcode > +xfs_validate_stripe_factors( > + int sectorsize, > + int *sup, Errant space between "int" and "*sup". > + int *swp); > + > #endif /* __LIBFROG_TOPOLOGY_H__ */ > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > index 2e6cd280..a3d6032c 100644 > --- a/mkfs/xfs_mkfs.c > +++ b/mkfs/xfs_mkfs.c > @@ -2255,7 +2255,6 @@ calc_stripe_factors( > struct cli_params *cli, > struct fs_topology *ft) > { > - long long int big_dswidth; > int dsunit = 0; > int dswidth = 0; > int lsunit = 0; > @@ -2263,6 +2262,7 @@ calc_stripe_factors( > int dsw = 0; > int lsu = 0; > bool use_dev = false; > + int error; > > if (cli_opt_set(&dopts, D_SUNIT)) > dsunit = cli->dsunit; > @@ -2289,31 +2289,40 @@ _("both data su and data sw options must be specified\n")); > usage(); > } > > - if (dsu % cfg->sectorsize) { > + dsunit = dsu; > + dswidth = dsw; > + error = xfs_validate_stripe_factors(cfg->sectorsize, &dsunit, &dswidth); I thought this function returned an enum? > + switch(error) { > + case XFS_STRIPE_RET_SUNIT_MISALIGN: > fprintf(stderr, > _("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize); > usage(); > - } > - > - dsunit = (int)BTOBBT(dsu); > - big_dswidth = (long long int)dsunit * dsw; > - if (big_dswidth > INT_MAX) { > + break; > + case XFS_STRIPE_RET_SWIDTH_OVERFLOW: > fprintf(stderr, > -_("data stripe width (%lld) is too large of a multiple of the data stripe unit (%d)\n"), > - big_dswidth, dsunit); > +_("data stripe width (dsw %d) is too large of a multiple of the data stripe unit (%d)\n"), Why change this message? > + dsw, dsunit); > usage(); > + break; > } > - dswidth = big_dswidth; > + } else { > + error = xfs_validate_stripe_factors(0, &dsunit, &dswidth); > } > > - if ((dsunit && !dswidth) || (!dsunit && dswidth) || > - (dsunit && (dswidth % dsunit != 0))) { > + if (error == XFS_STRIPE_RET_PARTIAL_VALID || > + error == XFS_STRIPE_RET_SWIDTH_MISALIGN) { > fprintf(stderr, > _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"), > dswidth, dsunit); > usage(); > } > > + if (error) { > + fprintf(stderr, > +_("invalid data stripe unit (%d), width (%d)\n"), dsunit, dswidth); Invalid how? We know the exact reason, so we should say so. --D > + usage(); > + } > + > /* If sunit & swidth were manually specified as 0, same as noalign */ > if ((cli_opt_set(&dopts, D_SUNIT) || cli_opt_set(&dopts, D_SU)) && > !dsunit && !dswidth) > @@ -2328,18 +2337,9 @@ _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"), > > /* if no stripe config set, use the device default */ > if (!dsunit) { > - /* Ignore nonsense from device. XXX add more validation */ > - if (ft->dsunit && ft->dswidth == 0) { > - fprintf(stderr, > -_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"), > - progname, BBTOB(ft->dsunit)); > - ft->dsunit = 0; > - ft->dswidth = 0; > - } else { > - dsunit = ft->dsunit; > - dswidth = ft->dswidth; > - use_dev = true; > - } > + dsunit = ft->dsunit; > + dswidth = ft->dswidth; > + use_dev = true; > } else { > /* check and warn if user-specified alignment is sub-optimal */ > if (ft->dsunit && ft->dsunit != dsunit) { > -- > 2.18.1 >
Hi Darrick, On Mon, Aug 03, 2020 at 08:26:09AM -0700, Darrick J. Wong wrote: > On Mon, Aug 03, 2020 at 08:50:18PM +0800, Gao Xiang wrote: > > Currently stripe unit/width checking logic is all over xfsprogs. > > So, refactor the same code snippet into a single validation helper > > xfs_validate_stripe_factors(), including: > > - integer overflows of either value > > - sunit and swidth alignment wrt sector size > > - if either sunit or swidth are zero, both should be zero > > - swidth must be a multiple of sunit > > > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > > --- > > > > This patch follows Darrick's original suggestion [1], yet I'm > > not sure if I'm doing the right thing or if something is still > > missing (e.g the meaning of six(ish) places)... So post it > > right now... > > > > TBH, especially all these naming and the helper location (whether > > in topology.c)...plus, click a dislike on calc_stripe_factors() > > itself... > > > > (Hopefully hear some advice about this... Thanks!) > > > > [1] https://lore.kernel.org/r/20200515204802.GO6714@magnolia > > > > libfrog/topology.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++ > > libfrog/topology.h | 15 ++++++++++++++ > > mkfs/xfs_mkfs.c | 48 ++++++++++++++++++++++---------------------- > > 3 files changed, 89 insertions(+), 24 deletions(-) > > > > diff --git a/libfrog/topology.c b/libfrog/topology.c > > index b1b470c9..cf56fb03 100644 > > --- a/libfrog/topology.c > > +++ b/libfrog/topology.c > > @@ -174,6 +174,41 @@ out: > > return ret; > > } > > > > +enum xfs_stripe_retcode > > +xfs_validate_stripe_factors( > > libfrog functions (and enums) should be prefixed with libfrog, not xfs. > > LIBFROG_STRIPEVAL_{OK,SUNIT_MISALIGN, etc.} > > > + int sectorsize, > > + int *sup, > > Errant space between "int" and "*sup". Ack. Sorry about that. > > > + int *swp) > > Strange that a validator function has out parameters... > > Also, uh, .... full names, please. see the reasons below... > > int *sunitp, > int *swidthp) > > (I'm vaguely wondering why we use signed ints here vs. unsigned, but > that isn't critical...) That is because I saw many previous "sunit/swidth" usage in the codebase by using "int" rather than "unsigned int". I don't have much tendency of this. (either form is ok with me since signed int is also enough here.) > > > +{ > > + int sunit = *sup, swidth = *swp; > > + > > + if (sectorsize) { > > + long long big_swidth; > > + > > + if (sunit % sectorsize) > > + return XFS_STRIPE_RET_SUNIT_MISALIGN; > > + > > + sunit = (int)BTOBBT(sunit); > > Hmm. On input, *sup is in units of bytes, but on output it can be in > units of 512b blocks? That is very surprising... Yeah, It seems a bit weird at first. But I have no better idea how to fulfill/wrap up "- sunit and swidth alignment wrt sector size" check from the original thread in the validator helper. So I finally implemented the helper in a form which accepts either: [1] (sectersize != 0) dsu (in bytes) / dsw (which is multiple of dsu) Or [2] (sectersize == 0) dunit / dwidth (in 512b sector size) In [1], dsu and dsw would be turned into dunit / dwidth finally... Yeah, that's my premature thought about this tho... hope for better idea about this :) > > > + big_swidth = (long long)sunit * swidth; > > + > > + if (big_swidth > INT_MAX) > > + return XFS_STRIPE_RET_SWIDTH_OVERFLOW; > > + swidth = big_swidth; > > + } > > + if ((sunit && !swidth) || (!sunit && swidth)) > > + return XFS_STRIPE_RET_PARTIAL_VALID; > > + > > + if (sunit > swidth) > > + return XFS_STRIPE_RET_SUNIT_TOO_LARGE; > > + > > + if (sunit && (swidth % sunit)) > > + return XFS_STRIPE_RET_SWIDTH_MISALIGN; > > + > > + *sup = sunit; > > ...especially since in the !sectorsize case we don't change it at all. Yeah... > > > + *swp = swidth; > > + return XFS_STRIPE_RET_OK; > > +} > > + > > static void blkid_get_topology( > > const char *device, > > int *sunit, > > @@ -229,6 +264,21 @@ static void blkid_get_topology( > > */ > > *sunit = *sunit >> 9; > > *swidth = *swidth >> 9; > > + switch (xfs_validate_stripe_factors(0, sunit, swidth)) { > > + case XFS_STRIPE_RET_OK: > > + break; > > + case XFS_STRIPE_RET_PARTIAL_VALID: > > + fprintf(stderr, > > +_("%s: Volume reports stripe unit of %d bytes and stripe width of %d bytes, ignoring.\n"), > > + progname, BBTOB(*sunit), BBTOB(*swidth)); > > Needs a "/* fallthrough */" comment here. Ack. > > > + default: > > Why don't we warn about receiving garbage geometry that produces > MISALIGN or OVERFLOW? Okay, I could add them in the next version... Yet I still suspect my broken English works... :) > > > + /* > > + * if firmware is broken, just give up and set both to zero, > > + * we can't trust information from this device. > > + */ > > + *sunit = 0; > > + *swidth = 0; > > + } > > > > if (blkid_topology_get_alignment_offset(tp) != 0) { > > fprintf(stderr, > > diff --git a/libfrog/topology.h b/libfrog/topology.h > > index 6fde868a..e8be26b2 100644 > > --- a/libfrog/topology.h > > +++ b/libfrog/topology.h > > @@ -36,4 +36,19 @@ extern int > > check_overwrite( > > const char *device); > > > > +enum xfs_stripe_retcode { > > + XFS_STRIPE_RET_OK = 0, > > + XFS_STRIPE_RET_SUNIT_MISALIGN, > > + XFS_STRIPE_RET_SWIDTH_OVERFLOW, > > + XFS_STRIPE_RET_PARTIAL_VALID, > > + XFS_STRIPE_RET_SUNIT_TOO_LARGE, > > + XFS_STRIPE_RET_SWIDTH_MISALIGN, > > +}; > > + > > +enum xfs_stripe_retcode > > +xfs_validate_stripe_factors( > > + int sectorsize, > > + int *sup, > > Errant space between "int" and "*sup". Sorry, copy again. > > > + int *swp); > > + > > #endif /* __LIBFROG_TOPOLOGY_H__ */ > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > > index 2e6cd280..a3d6032c 100644 > > --- a/mkfs/xfs_mkfs.c > > +++ b/mkfs/xfs_mkfs.c > > @@ -2255,7 +2255,6 @@ calc_stripe_factors( > > struct cli_params *cli, > > struct fs_topology *ft) > > { > > - long long int big_dswidth; > > int dsunit = 0; > > int dswidth = 0; > > int lsunit = 0; > > @@ -2263,6 +2262,7 @@ calc_stripe_factors( > > int dsw = 0; > > int lsu = 0; > > bool use_dev = false; > > + int error; > > > > if (cli_opt_set(&dopts, D_SUNIT)) > > dsunit = cli->dsunit; > > @@ -2289,31 +2289,40 @@ _("both data su and data sw options must be specified\n")); > > usage(); > > } > > > > - if (dsu % cfg->sectorsize) { > > + dsunit = dsu; > > + dswidth = dsw; > > + error = xfs_validate_stripe_factors(cfg->sectorsize, &dsunit, &dswidth); > > I thought this function returned an enum? okay, will update in the next version. > > > + switch(error) { > > + case XFS_STRIPE_RET_SUNIT_MISALIGN: > > fprintf(stderr, > > _("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize); > > usage(); > > - } > > - > > - dsunit = (int)BTOBBT(dsu); > > - big_dswidth = (long long int)dsunit * dsw; > > - if (big_dswidth > INT_MAX) { > > + break; > > + case XFS_STRIPE_RET_SWIDTH_OVERFLOW: > > fprintf(stderr, > > -_("data stripe width (%lld) is too large of a multiple of the data stripe unit (%d)\n"), > > - big_dswidth, dsunit); > > +_("data stripe width (dsw %d) is too large of a multiple of the data stripe unit (%d)\n"), > > Why change this message? big_dswidth isn't defined here. So I'm not sure if the original message can still be properly used here. (I could leave it alone...) > > > + dsw, dsunit); > > usage(); > > + break; > > } > > - dswidth = big_dswidth; > > + } else { > > + error = xfs_validate_stripe_factors(0, &dsunit, &dswidth); > > } > > > > - if ((dsunit && !dswidth) || (!dsunit && dswidth) || > > - (dsunit && (dswidth % dsunit != 0))) { > > + if (error == XFS_STRIPE_RET_PARTIAL_VALID || > > + error == XFS_STRIPE_RET_SWIDTH_MISALIGN) { > > fprintf(stderr, > > _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"), > > dswidth, dsunit); > > usage(); > > } > > > > + if (error) { > > + fprintf(stderr, > > +_("invalid data stripe unit (%d), width (%d)\n"), dsunit, dswidth); > > Invalid how? We know the exact reason, so we should say so. Okay, let me think more about some cleaner way for these message. I feel it could be a bit messy here. Thanks， Gao Xiang > > --D > > > + usage(); > > + } > > + > > /* If sunit & swidth were manually specified as 0, same as noalign */ > > if ((cli_opt_set(&dopts, D_SUNIT) || cli_opt_set(&dopts, D_SU)) && > > !dsunit && !dswidth) > > @@ -2328,18 +2337,9 @@ _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"), > > > > /* if no stripe config set, use the device default */ > > if (!dsunit) { > > - /* Ignore nonsense from device. XXX add more validation */ > > - if (ft->dsunit && ft->dswidth == 0) { > > - fprintf(stderr, > > -_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"), > > - progname, BBTOB(ft->dsunit)); > > - ft->dsunit = 0; > > - ft->dswidth = 0; > > - } else { > > - dsunit = ft->dsunit; > > - dswidth = ft->dswidth; > > - use_dev = true; > > - } > > + dsunit = ft->dsunit; > > + dswidth = ft->dswidth; > > + use_dev = true; > > } else { > > /* check and warn if user-specified alignment is sub-optimal */ > > if (ft->dsunit && ft->dsunit != dsunit) { > > -- > > 2.18.1 > > >
diff --git a/libfrog/topology.c b/libfrog/topology.c index b1b470c9..cf56fb03 100644 --- a/libfrog/topology.c +++ b/libfrog/topology.c @@ -174,6 +174,41 @@ out: return ret; } +enum xfs_stripe_retcode +xfs_validate_stripe_factors( + int sectorsize, + int *sup, + int *swp) +{ + int sunit = *sup, swidth = *swp; + + if (sectorsize) { + long long big_swidth; + + if (sunit % sectorsize) + return XFS_STRIPE_RET_SUNIT_MISALIGN; + + sunit = (int)BTOBBT(sunit); + big_swidth = (long long)sunit * swidth; + + if (big_swidth > INT_MAX) + return XFS_STRIPE_RET_SWIDTH_OVERFLOW; + swidth = big_swidth; + } + if ((sunit && !swidth) || (!sunit && swidth)) + return XFS_STRIPE_RET_PARTIAL_VALID; + + if (sunit > swidth) + return XFS_STRIPE_RET_SUNIT_TOO_LARGE; + + if (sunit && (swidth % sunit)) + return XFS_STRIPE_RET_SWIDTH_MISALIGN; + + *sup = sunit; + *swp = swidth; + return XFS_STRIPE_RET_OK; +} + static void blkid_get_topology( const char *device, int *sunit, @@ -229,6 +264,21 @@ static void blkid_get_topology( */ *sunit = *sunit >> 9; *swidth = *swidth >> 9; + switch (xfs_validate_stripe_factors(0, sunit, swidth)) { + case XFS_STRIPE_RET_OK: + break; + case XFS_STRIPE_RET_PARTIAL_VALID: + fprintf(stderr, +_("%s: Volume reports stripe unit of %d bytes and stripe width of %d bytes, ignoring.\n"), + progname, BBTOB(*sunit), BBTOB(*swidth)); + default: + /* + * if firmware is broken, just give up and set both to zero, + * we can't trust information from this device. + */ + *sunit = 0; + *swidth = 0; + } if (blkid_topology_get_alignment_offset(tp) != 0) { fprintf(stderr, diff --git a/libfrog/topology.h b/libfrog/topology.h index 6fde868a..e8be26b2 100644 --- a/libfrog/topology.h +++ b/libfrog/topology.h @@ -36,4 +36,19 @@ extern int check_overwrite( const char *device); +enum xfs_stripe_retcode { + XFS_STRIPE_RET_OK = 0, + XFS_STRIPE_RET_SUNIT_MISALIGN, + XFS_STRIPE_RET_SWIDTH_OVERFLOW, + XFS_STRIPE_RET_PARTIAL_VALID, + XFS_STRIPE_RET_SUNIT_TOO_LARGE, + XFS_STRIPE_RET_SWIDTH_MISALIGN, +}; + +enum xfs_stripe_retcode +xfs_validate_stripe_factors( + int sectorsize, + int *sup, + int *swp); + #endif /* __LIBFROG_TOPOLOGY_H__ */ diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c index 2e6cd280..a3d6032c 100644 --- a/mkfs/xfs_mkfs.c +++ b/mkfs/xfs_mkfs.c @@ -2255,7 +2255,6 @@ calc_stripe_factors( struct cli_params *cli, struct fs_topology *ft) { - long long int big_dswidth; int dsunit = 0; int dswidth = 0; int lsunit = 0; @@ -2263,6 +2262,7 @@ calc_stripe_factors( int dsw = 0; int lsu = 0; bool use_dev = false; + int error; if (cli_opt_set(&dopts, D_SUNIT)) dsunit = cli->dsunit; @@ -2289,31 +2289,40 @@ _("both data su and data sw options must be specified\n")); usage(); } - if (dsu % cfg->sectorsize) { + dsunit = dsu; + dswidth = dsw; + error = xfs_validate_stripe_factors(cfg->sectorsize, &dsunit, &dswidth); + switch(error) { + case XFS_STRIPE_RET_SUNIT_MISALIGN: fprintf(stderr, _("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize); usage(); - } - - dsunit = (int)BTOBBT(dsu); - big_dswidth = (long long int)dsunit * dsw; - if (big_dswidth > INT_MAX) { + break; + case XFS_STRIPE_RET_SWIDTH_OVERFLOW: fprintf(stderr, -_("data stripe width (%lld) is too large of a multiple of the data stripe unit (%d)\n"), - big_dswidth, dsunit); +_("data stripe width (dsw %d) is too large of a multiple of the data stripe unit (%d)\n"), + dsw, dsunit); usage(); + break; } - dswidth = big_dswidth; + } else { + error = xfs_validate_stripe_factors(0, &dsunit, &dswidth); } - if ((dsunit && !dswidth) || (!dsunit && dswidth) || - (dsunit && (dswidth % dsunit != 0))) { + if (error == XFS_STRIPE_RET_PARTIAL_VALID || + error == XFS_STRIPE_RET_SWIDTH_MISALIGN) { fprintf(stderr, _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"), dswidth, dsunit); usage(); } + if (error) { + fprintf(stderr, +_("invalid data stripe unit (%d), width (%d)\n"), dsunit, dswidth); + usage(); + } + /* If sunit & swidth were manually specified as 0, same as noalign */ if ((cli_opt_set(&dopts, D_SUNIT) || cli_opt_set(&dopts, D_SU)) && !dsunit && !dswidth) @@ -2328,18 +2337,9 @@ _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"), /* if no stripe config set, use the device default */ if (!dsunit) { - /* Ignore nonsense from device. XXX add more validation */ - if (ft->dsunit && ft->dswidth == 0) { - fprintf(stderr, -_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"), - progname, BBTOB(ft->dsunit)); - ft->dsunit = 0; - ft->dswidth = 0; - } else { - dsunit = ft->dsunit; - dswidth = ft->dswidth; - use_dev = true; - } + dsunit = ft->dsunit; + dswidth = ft->dswidth; + use_dev = true; } else { /* check and warn if user-specified alignment is sub-optimal */ if (ft->dsunit && ft->dsunit != dsunit) {
Currently stripe unit/width checking logic is all over xfsprogs. So, refactor the same code snippet into a single validation helper xfs_validate_stripe_factors(), including: - integer overflows of either value - sunit and swidth alignment wrt sector size - if either sunit or swidth are zero, both should be zero - swidth must be a multiple of sunit Signed-off-by: Gao Xiang <hsiangkao@redhat.com> --- This patch follows Darrick's original suggestion [1], yet I'm not sure if I'm doing the right thing or if something is still missing (e.g the meaning of six(ish) places)... So post it right now... TBH, especially all these naming and the helper location (whether in topology.c)...plus, click a dislike on calc_stripe_factors() itself... (Hopefully hear some advice about this... Thanks!) [1] https://lore.kernel.org/r/20200515204802.GO6714@magnolia libfrog/topology.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++ libfrog/topology.h | 15 ++++++++++++++ mkfs/xfs_mkfs.c | 48 ++++++++++++++++++++++---------------------- 3 files changed, 89 insertions(+), 24 deletions(-)