Message ID | 20201007140402.14295-4-hsiangkao@aol.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | xfsprogs: consolidate stripe validation | expand |
On Wed, Oct 07, 2020 at 10:04:02PM +0800, Gao Xiang wrote: > From: Gao Xiang <hsiangkao@redhat.com> > > Check stripe numbers in calc_stripe_factors() by > using xfs_validate_stripe_factors(). > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > --- > mkfs/xfs_mkfs.c | 23 +++++++---------------- > 1 file changed, 7 insertions(+), 16 deletions(-) > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > index 2e6cd280e388..b7f8f98147eb 100644 > --- a/mkfs/xfs_mkfs.c > +++ b/mkfs/xfs_mkfs.c > @@ -2289,12 +2289,6 @@ _("both data su and data sw options must be specified\n")); > usage(); > } > > - if (dsu % cfg->sectorsize) { > - 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) { > @@ -2306,13 +2300,9 @@ _("data stripe width (%lld) is too large of a multiple of the data stripe unit ( > dswidth = big_dswidth; > } > > - if ((dsunit && !dswidth) || (!dsunit && dswidth) || > - (dsunit && (dswidth % dsunit != 0))) { > - fprintf(stderr, > -_("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"), > - dswidth, dsunit); > + if (!xfs_validate_stripe_factors(NULL, BBTOB(dsunit), BBTOB(dswidth), if (!libxfs_validate_stripe_factors(...)) Unless we get rid of the weird libxfs macro thing, you're supposed to use prefixes in userspace. --D > + cfg->sectorsize)) > 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)) && > @@ -2328,11 +2318,12 @@ _("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) { > + /* Ignore nonsense from device report. */ > + if (!xfs_validate_stripe_factors(NULL, 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)); > +_("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\n"), > + progname, BBTOB(ft->dsunit), BBTOB(ft->dswidth)); > ft->dsunit = 0; > ft->dswidth = 0; > } else { > -- > 2.24.0 >
On Wed, Oct 07, 2020 at 03:30:44PM -0700, Darrick J. Wong wrote: > On Wed, Oct 07, 2020 at 10:04:02PM +0800, Gao Xiang wrote: > > From: Gao Xiang <hsiangkao@redhat.com> > > > > Check stripe numbers in calc_stripe_factors() by > > using xfs_validate_stripe_factors(). > > > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > > --- > > mkfs/xfs_mkfs.c | 23 +++++++---------------- > > 1 file changed, 7 insertions(+), 16 deletions(-) > > > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > > index 2e6cd280e388..b7f8f98147eb 100644 > > --- a/mkfs/xfs_mkfs.c > > +++ b/mkfs/xfs_mkfs.c > > @@ -2289,12 +2289,6 @@ _("both data su and data sw options must be specified\n")); > > usage(); > > } > > > > - if (dsu % cfg->sectorsize) { > > - 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) { > > @@ -2306,13 +2300,9 @@ _("data stripe width (%lld) is too large of a multiple of the data stripe unit ( > > dswidth = big_dswidth; > > } > > > > - if ((dsunit && !dswidth) || (!dsunit && dswidth) || > > - (dsunit && (dswidth % dsunit != 0))) { > > - fprintf(stderr, > > -_("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"), > > - dswidth, dsunit); > > + if (!xfs_validate_stripe_factors(NULL, BBTOB(dsunit), BBTOB(dswidth), > > if (!libxfs_validate_stripe_factors(...)) > > Unless we get rid of the weird libxfs macro thing, you're supposed to > use prefixes in userspace. I vaguely remembered Christoph sent out a patch intending to get rid of xfsprogs libxfs_ prefix months ago, so I assumed there was no need to introduce any new libxfs_ userspace API wrappers anymore. But yeah, will add such libxfs_ marco wrapper in the next version. Thanks, Gao Xiang > > --D
On 10/8/20 7:58 PM, Gao Xiang wrote: >> Unless we get rid of the weird libxfs macro thing, you're supposed to >> use prefixes in userspace. > I vaguely remembered Christoph sent out a patch intending to get > rid of xfsprogs libxfs_ prefix months ago, so I assumed there was > no need to introduce any new libxfs_ userspace API wrappers anymore. He did, and it's on my (perpetual) TODO list to get that finally reviewed, sorry. For now we still have libxfs* -Eric > But yeah, will add such libxfs_ marco wrapper in the next version. > > Thanks, > Gao Xiang >
On Fri, Oct 09, 2020 at 08:02:02AM -0500, Eric Sandeen wrote: > On 10/8/20 7:58 PM, Gao Xiang wrote: > >> Unless we get rid of the weird libxfs macro thing, you're supposed to > >> use prefixes in userspace. > > I vaguely remembered Christoph sent out a patch intending to get > > rid of xfsprogs libxfs_ prefix months ago, so I assumed there was > > no need to introduce any new libxfs_ userspace API wrappers anymore. > > He did, and it's on my (perpetual) TODO list to get that finally reviewed, > sorry. > > For now we still have libxfs* Yeah, I've sent out the next version with libxfs_ prefix at https://lore.kernel.org/r/20201009052421.3328-4-hsiangkao@redhat.com Thanks for the information as well! Thanks, Gao Xiang > > -Eric > > > But yeah, will add such libxfs_ marco wrapper in the next version. > > > > Thanks, > > Gao Xiang > > >
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c index 2e6cd280e388..b7f8f98147eb 100644 --- a/mkfs/xfs_mkfs.c +++ b/mkfs/xfs_mkfs.c @@ -2289,12 +2289,6 @@ _("both data su and data sw options must be specified\n")); usage(); } - if (dsu % cfg->sectorsize) { - 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) { @@ -2306,13 +2300,9 @@ _("data stripe width (%lld) is too large of a multiple of the data stripe unit ( dswidth = big_dswidth; } - if ((dsunit && !dswidth) || (!dsunit && dswidth) || - (dsunit && (dswidth % dsunit != 0))) { - fprintf(stderr, -_("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"), - dswidth, dsunit); + if (!xfs_validate_stripe_factors(NULL, BBTOB(dsunit), BBTOB(dswidth), + cfg->sectorsize)) 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)) && @@ -2328,11 +2318,12 @@ _("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) { + /* Ignore nonsense from device report. */ + if (!xfs_validate_stripe_factors(NULL, 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)); +_("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\n"), + progname, BBTOB(ft->dsunit), BBTOB(ft->dswidth)); ft->dsunit = 0; ft->dswidth = 0; } else {