Message ID | 20201009052421.3328-4-hsiangkao@redhat.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | xfsprogs: consolidate stripe validation | expand |
On Fri, Oct 09, 2020 at 01:24:21PM +0800, Gao Xiang wrote: > Check stripe numbers in calc_stripe_factors() by using > xfs_validate_stripe_factors(). > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > --- > libxfs/libxfs_api_defs.h | 1 + > mkfs/xfs_mkfs.c | 23 +++++++---------------- > 2 files changed, 8 insertions(+), 16 deletions(-) > ... > @@ -2344,11 +2334,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 (!libxfs_validate_stripe_factors(NULL, BBTOB(ft->dsunit), > + BBTOB(ft->dswidth), 0)) { The logic seems fine and from the previous comment it sounds like we're lacking validation in this particular scenario, but do we want to print more error noise from the validation helper in scenarios where failure is not a fatal error? Brian > 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.18.1 >
Hi Brian, On Mon, Oct 12, 2020 at 09:06:51AM -0400, Brian Foster wrote: > On Fri, Oct 09, 2020 at 01:24:21PM +0800, Gao Xiang wrote: > > Check stripe numbers in calc_stripe_factors() by using > > xfs_validate_stripe_factors(). > > > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > > --- > > libxfs/libxfs_api_defs.h | 1 + > > mkfs/xfs_mkfs.c | 23 +++++++---------------- > > 2 files changed, 8 insertions(+), 16 deletions(-) > > > ... > > @@ -2344,11 +2334,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 (!libxfs_validate_stripe_factors(NULL, BBTOB(ft->dsunit), > > + BBTOB(ft->dswidth), 0)) { > > The logic seems fine and from the previous comment it sounds like we're > lacking validation in this particular scenario, but do we want to print > more error noise from the validation helper in scenarios where failure > is not a fatal error? Yeah, If I understand correctly, I think that is an open question here, so I think you suggested that we could silence for this case by passing a "bool silent" argument? or some better idea for this? Thanks, Gao Xiang > > Brian > > > 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.18.1 > > >
On Mon, Oct 12, 2020 at 10:07:15PM +0800, Gao Xiang wrote: > Hi Brian, > > On Mon, Oct 12, 2020 at 09:06:51AM -0400, Brian Foster wrote: > > On Fri, Oct 09, 2020 at 01:24:21PM +0800, Gao Xiang wrote: > > > Check stripe numbers in calc_stripe_factors() by using > > > xfs_validate_stripe_factors(). > > > > > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > > > --- > > > libxfs/libxfs_api_defs.h | 1 + > > > mkfs/xfs_mkfs.c | 23 +++++++---------------- > > > 2 files changed, 8 insertions(+), 16 deletions(-) > > > > > ... > > > @@ -2344,11 +2334,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 (!libxfs_validate_stripe_factors(NULL, BBTOB(ft->dsunit), > > > + BBTOB(ft->dswidth), 0)) { > > > > The logic seems fine and from the previous comment it sounds like we're > > lacking validation in this particular scenario, but do we want to print > > more error noise from the validation helper in scenarios where failure > > is not a fatal error? > > Yeah, If I understand correctly, I think that is an open question here, > so I think you suggested that we could silence for this case by passing > a "bool silent" argument? or some better idea for this? > Sure, that seems reasonable to me. Maybe others have thoughts. My concern was primarily based on usability in terms of not potentially confusing users with spurious error messages. Brian > Thanks, > Gao Xiang > > > > > Brian > > > > > 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.18.1 > > > > > >
diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h index e7e42e93..c1b009c1 100644 --- a/libxfs/libxfs_api_defs.h +++ b/libxfs/libxfs_api_defs.h @@ -188,6 +188,7 @@ #define xfs_trans_roll_inode libxfs_trans_roll_inode #define xfs_trans_roll libxfs_trans_roll +#define xfs_validate_stripe_factors libxfs_validate_stripe_factors #define xfs_verify_agbno libxfs_verify_agbno #define xfs_verify_agino libxfs_verify_agino #define xfs_verify_cksum libxfs_verify_cksum diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c index 8fe149d7..5ce063ae 100644 --- a/mkfs/xfs_mkfs.c +++ b/mkfs/xfs_mkfs.c @@ -2305,12 +2305,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) { @@ -2322,13 +2316,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 (!libxfs_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)) && @@ -2344,11 +2334,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 (!libxfs_validate_stripe_factors(NULL, BBTOB(ft->dsunit), + BBTOB(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 {
Check stripe numbers in calc_stripe_factors() by using xfs_validate_stripe_factors(). Signed-off-by: Gao Xiang <hsiangkao@redhat.com> --- libxfs/libxfs_api_defs.h | 1 + mkfs/xfs_mkfs.c | 23 +++++++---------------- 2 files changed, 8 insertions(+), 16 deletions(-)