On Fri, May 15, 2020 at 02:14:17PM -0500, Eric Sandeen wrote:
> We validate commandline options for stripe unit and stripe width, and
> if a device returns nonsensical values via libblkid, the superbock write
> verifier will eventually catch it and fail (noisily and cryptically) but
> it seems a bit cleaner to just do a basic sanity check on the numbers
> as soon as we get them from blkid, and if they're bogus, ignore them from
> the start.
>
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> ---
>
> diff --git a/libfrog/topology.c b/libfrog/topology.c
> index b1b470c9..38ed03b7 100644
> --- a/libfrog/topology.c
> +++ b/libfrog/topology.c
> @@ -213,6 +213,19 @@ static void blkid_get_topology(
> val = blkid_topology_get_optimal_io_size(tp);
> *swidth = val;
>
> + /*
> + * Occasionally, firmware is broken and returns optimal < minimum,
> + * or optimal which is not a multiple of minimum.
> + * In that case just give up and set both to zero, we can't trust
> + * information from this device. Similar to xfs_validate_sb_common().
> + */
> + if (*sunit) {
> + if ((*sunit > *swidth) || (*swidth % *sunit != 0)) {
I feel like we're copypasting this sunit/swidth checking logic all over
xfsprogs and yet we're still losing the stripe unit validation whackamole
game.
In the end, we want to check more or less the same things for each pair
of stripe unit and stripe width:
* 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
All four of these rules apply to the blkid_get_toplogy answers for the
data device, the log device, and the realtime device; and any mkfs CLI
overrides of those values.
IOWs, is there some way to refactor those four rules into a single
validation function and call that in the six(ish) places we need it?
Especially since you're the one who played the last round of whackamole,
back in May 2018. :)
--D
> + *sunit = 0;
> + *swidth = 0;
> + }
> + }
> +
> /*
> * If the reported values are the same as the physical sector size
> * do not bother to report anything. It will only cause warnings
>
