Hi Eric,
On Tue, Aug 04, 2020 at 12:55:43PM -0700, Eric Sandeen wrote:
> On 8/4/20 9:00 AM, Gao Xiang wrote:
> > Currently stripe unit/swidth 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>
> > ---
> > changes since v1:
> > - several update (errant space, full names...) suggested by Darrick;
> > - rearrange into a unique handler in calc_stripe_factors();
> > - add libfrog_stripeval_str[] yet I'm not sure if it needs localization;
> > - update po translalation due to (%lld type -> %d).
> >
> > (I'd still like to post it in advance...)
>
> Sorry for not commenting sooner.
>
> I wonder - would it be possible to factor out a stripe value validation
> helper from xfs_validate_sb_common() in libxfs/xfs_sb.c, so that this
> could be called from userspace too?
>
> It is a bit different because kernelspace checks against whether the
> superblock has XFS_SB_VERSION_DALIGNBIT set, and that makes no sense
> in i.e. blkid_get_topology.
Ah, sorry for not noticing that code snippet.
I think dalign check could be outside this helper since it doesn't
need to be considered for the other userspace callers (e.g. if considering
passing in it, it seems needing 2 extra arguments (has_dalign_check and
isdalign and it seems uncessary)?
maybe such condition can be simplified in a line in the
xfs_validate_sb_common() as
if (!sbp->sb_unit ^ !xfs_sb_version_hasdalign(sbp)) {
...
return -FSCURRUPTTED;
}
>
> On the other hand, the code below currently checks against sector size,
> which seems to be something that kernelspace does not do currently
> (but it probably could).
It seems that it doesn't matter since we could pass (sectorsize == 0)
and use sunit/swidth rather than the specfic dsu/dsw argument approach
to skip the related check.
>
> Doing all of this checking in a common location in libxfs for
> both userspace and kernelspace seems like it would be a good goal.
I will try to fold xfs_validate_sb_common() case (in xfsprogs first
for review), the prefix should be xfs_ then?
Thanks,
Gao Xiang
>
> Thoughts?
>
> -Eric
>
> > libfrog/topology.c | 68 +++++++++++++++++++++++++++++++++++++++++
> > libfrog/topology.h | 17 +++++++++++
> > mkfs/xfs_mkfs.c | 76 ++++++++++++++++++++++++----------------------
> > po/pl.po | 4 +--
> > 4 files changed, 126 insertions(+), 39 deletions(-)
> >
> > diff --git a/libfrog/topology.c b/libfrog/topology.c
> > index b1b470c9..1ce151fd 100644
> > --- a/libfrog/topology.c
> > +++ b/libfrog/topology.c
> > @@ -174,6 +174,59 @@ out:
> > return ret;
> > }
> >
> > +/*
> > + * This accepts either
> > + * - (sectersize != 0) dsu (in bytes) / dsw (which is mulplier of dsu)
> > + * or
> > + * - (sectersize == 0) dunit / dwidth (in 512b sector size)
> > + * and return sunit/swidth in sectors.
> > + */
> > +enum libfrog_stripeval
> > +libfrog_validate_stripe_factors(
> > + int sectorsize,
> > + int *sunitp,
> > + int *swidthp)
> > +{
> > + int sunit = *sunitp;
> > + int swidth = *swidthp;
> > +
> > + if (sectorsize) {
> > + long long big_swidth;
> > +
> > + if (sunit % sectorsize)
> > + return LIBFROG_STRIPEVAL_SUNIT_MISALIGN;
> > +
> > + sunit = (int)BTOBBT(sunit);
> > + big_swidth = (long long)sunit * swidth;
> > +
> > + if (big_swidth > INT_MAX)
> > + return LIBFROG_STRIPEVAL_SWIDTH_OVERFLOW;
> > + swidth = big_swidth;
> > + }
> > +
> > + if ((sunit && !swidth) || (!sunit && swidth))
> > + return LIBFROG_STRIPEVAL_PARTIAL_VALID;
> > +
> > + if (sunit > swidth)
> > + return LIBFROG_STRIPEVAL_SUNIT_TOO_LARGE;
> > +
> > + if (sunit && (swidth % sunit))
> > + return LIBFROG_STRIPEVAL_SWIDTH_MISALIGN;
> > +
> > + *sunitp = sunit;
> > + *swidthp = swidth;
> > + return LIBFROG_STRIPEVAL_OK;
> > +}
> > +
> > +const char *libfrog_stripeval_str[] = {
> > + "OK",
> > + "SUNIT_MISALIGN",
> > + "SWIDTH_OVERFLOW",
> > + "PARTIAL_VALID",
> > + "SUNIT_TOO_LARGE",
> > + "SWIDTH_MISALIGN",
> > +};
> > +
> > static void blkid_get_topology(
> > const char *device,
> > int *sunit,
> > @@ -187,6 +240,7 @@ static void blkid_get_topology(
> > blkid_probe pr;
> > unsigned long val;
> > struct stat statbuf;
> > + enum libfrog_stripeval error;
> >
> > /* can't get topology info from a file */
> > if (!stat(device, &statbuf) && S_ISREG(statbuf.st_mode)) {
> > @@ -230,6 +284,20 @@ static void blkid_get_topology(
> > *sunit = *sunit >> 9;
> > *swidth = *swidth >> 9;
> >
> > + error = libfrog_validate_stripe_factors(0, sunit, swidth);
> > + if (error) {
> > + fprintf(stderr,
> > +_("%s: Volume reports invalid sunit (%d bytes) and swidth (%d bytes) %s, ignoring.\n"),
> > + progname, BBTOB(*sunit), BBTOB(*swidth),
> > + libfrog_stripeval_str[error]);
> > + /*
> > + * 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,
> > _("warning: device is not properly aligned %s\n"),
> > diff --git a/libfrog/topology.h b/libfrog/topology.h
> > index 6fde868a..507fe121 100644
> > --- a/libfrog/topology.h
> > +++ b/libfrog/topology.h
> > @@ -36,4 +36,21 @@ extern int
> > check_overwrite(
> > const char *device);
> >
> > +enum libfrog_stripeval {
> > + LIBFROG_STRIPEVAL_OK = 0,
> > + LIBFROG_STRIPEVAL_SUNIT_MISALIGN,
> > + LIBFROG_STRIPEVAL_SWIDTH_OVERFLOW,
> > + LIBFROG_STRIPEVAL_PARTIAL_VALID,
> > + LIBFROG_STRIPEVAL_SUNIT_TOO_LARGE,
> > + LIBFROG_STRIPEVAL_SWIDTH_MISALIGN,
> > +};
> > +
> > +extern const char *libfrog_stripeval_str[];
> > +
> > +enum libfrog_stripeval
> > +libfrog_validate_stripe_factors(
> > + int sectorsize,
> > + int *sunitp,
> > + int *swidthp);
> > +
> > #endif /* __LIBFROG_TOPOLOGY_H__ */
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index 2e6cd280..f7b38b36 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -2255,14 +2255,14 @@ 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;
> > - int dsu = 0;
> > - int dsw = 0;
> > - int lsu = 0;
> > - bool use_dev = false;
> > + int dsunit = 0;
> > + int dswidth = 0;
> > + int lsunit = 0;
> > + int dsu = 0;
> > + int dsw = 0;
> > + int lsu = 0;
> > + bool use_dev = false;
> > + enum libfrog_stripeval error;
> >
> > if (cli_opt_set(&dopts, D_SUNIT))
> > dsunit = cli->dsunit;
> > @@ -2289,29 +2289,40 @@ _("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) {
> > - fprintf(stderr,
> > -_("data stripe width (%lld) is too large of a multiple of the data stripe unit (%d)\n"),
> > - big_dswidth, dsunit);
> > - usage();
> > - }
> > - dswidth = big_dswidth;
> > + dsunit = dsu;
> > + dswidth = dsw;
> > + error = libfrog_validate_stripe_factors(cfg->sectorsize,
> > + &dsunit, &dswidth);
> > + } else {
> > + error = libfrog_validate_stripe_factors(0, &dsunit, &dswidth);
> > }
> >
> > - if ((dsunit && !dswidth) || (!dsunit && dswidth) ||
> > - (dsunit && (dswidth % dsunit != 0))) {
> > + switch (error) {
> > + case LIBFROG_STRIPEVAL_OK:
> > + break;
> > + case LIBFROG_STRIPEVAL_SUNIT_MISALIGN:
> > + fprintf(stderr,
> > +_("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize);
> > + usage();
> > + break;
> > + case LIBFROG_STRIPEVAL_SWIDTH_OVERFLOW:
> > + fprintf(stderr,
> > +_("data stripe width (%d) is too large of a multiple of the data stripe unit (%d)\n"),
> > + dsw, dsunit);
> > + usage();
> > + break;
> > + case LIBFROG_STRIPEVAL_PARTIAL_VALID:
> > + case LIBFROG_STRIPEVAL_SWIDTH_MISALIGN:
> > fprintf(stderr,
> > _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
> > dswidth, dsunit);
> > usage();
> > + break;
> > + default:
> > + fprintf(stderr,
> > +_("invalid data stripe unit (%d), width (%d) %s\n"),
> > + dsunit, dswidth, libfrog_stripeval_str[error]);
> > + usage();
> > }
> >
> > /* If sunit & swidth were manually specified as 0, same as noalign */
> > @@ -2328,18 +2339,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) {
> > diff --git a/po/pl.po b/po/pl.po
> > index 87109f6b..02d2258f 100644
> > --- a/po/pl.po
> > +++ b/po/pl.po
> > @@ -9085,10 +9085,10 @@ msgstr "su danych musi być wielokrotnością rozmiaru sektora (%d)\n"
> > #: .././mkfs/xfs_mkfs.c:2267
> > #, c-format
> > msgid ""
> > -"data stripe width (%lld) is too large of a multiple of the data stripe unit "
> > +"data stripe width (%d) is too large of a multiple of the data stripe unit "
> > "(%d)\n"
> > msgstr ""
> > -"szerokość pasa danych (%lld) jest zbyt dużą wielokrotnością jednostki pasa "
> > +"szerokość pasa danych (%d) jest zbyt dużą wielokrotnością jednostki pasa "
> > "danych (%d)\n"
> >
> > #: .././mkfs/xfs_mkfs.c:2276
> >
>
