Hi Eric,
On Mon, Sep 28, 2020 at 06:18:37PM -0500, Eric Sandeen wrote:
> On 8/6/20 8:03 AM, Gao Xiang wrote:
...
> > mkfs/xfs_mkfs.c | 62 ++++++++++++++++++++++++----------------------
> > po/pl.po | 4 +--
> > 5 files changed, 124 insertions(+), 43 deletions(-)
>
> Sorry this sat w/o review for a while... I'm going to kind of suggest a new
> approach here, since this seems to have gotten rather complicated.
Yeah, that is fine.
>
> > +/*
> > + * This accepts either
> > + * - (sectersize != 0) dsu (in bytes) / dsw (which is multiplier of dsu)
> > + * or
> > + * - (sectersize == 0) sunit / swidth (in 512B sectors)
> > + * and return sunit/swidth in sectors.
> > + */
>
> I'm still troubled by the complicated behavior of this function, even if it's
> fully described in the comment.
>
> What if this function:
>
> * only accepts bytes, and does not convert sectors <-> bytes
> - i.e. callers should convert to bytes first
>
> * directly prints the error using xfs_notice/warn() and an i8n'd _("...") string
> - this gets rid of enums & cases for strings, etc
> - kernelspace may need a #define to handle _("...")
>
> * becomes a boolean and returns true (geom ok) or false (geom bad)
> - caller can print more context if needed, i.e. "Device returned bad geometry..."
>
> * sectorsize check could be optional if we are calling from somewhere that
> does not need to or cannot validate against sector size (?)
>
I'm fine with the above proposal. I'll figout out a new patch
for this later (I'm outside now, will seek time).
The reason why it used enum here was to follow Darrick's
original suggestion in
https://lore.kernel.org/linux-xfs/20200515211011.GP6714@magnolia/
For such wrappers, Dave suggested updating xfs_notice() / xfs_warn()
in libxfs/libxfs_priv.h on IRC. So I will go on in that way.
If all people agree on this approach, I'm fine with all of that.
Thanks,
Gao Xiang
