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@xxxxxxxxxx> > > --- > > > > 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 > > >
