Re: [PATCH 04/12] mkfs: merge tables for opts parsing into one table

["Luis R. Rodriguez" <mcgrof@kernel.org>]

On Tue, Apr 25, 2017 at 08:28:23AM -0500, Eric Sandeen wrote:
> On 4/25/17 2:45 AM, Jan Tulak wrote:
> > On Tue, Apr 25, 2017 at 5:04 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> >> On Sun, Apr 23, 2017 at 08:54:55PM +0200, Jan Tulak wrote:
> >>> Merge separate instances of opt_params into one indexable table. Git
> >>> makes this patch looks a bit more complicated, but it does not change
> >>> values or structure of anything else. It only moves all the "struct
> >>> opt_params dopts = {...}", changes indentation for these substructures
> >>> and replaces their usage (dopts -> opts[OPT_D]).
> >>>
> >>> Signed-off-by: Jan Tulak <jtulak@redhat.com>
> >>> ---
> >>>  mkfs/xfs_mkfs.c | 1316 +++++++++++++++++++++++++++++--------------------------
> >>>  1 file changed, 683 insertions(+), 633 deletions(-)
> >>>
> >>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> >>> index 7a72b11..513e106 100644
> >>> --- a/mkfs/xfs_mkfs.c
> >>> +++ b/mkfs/xfs_mkfs.c
> >>> @@ -42,6 +42,7 @@ static int  ispow2(unsigned int i);
> >>>  uint64_t             blocksize;
> >>>  uint64_t             sectorsize;
> >>>
> >>> +#define MAX_OPTS     16
> >>
> >> This is fragile, every time a new opt is added this needs to be updated
> 
> <pedantic>There are only 8 now, so there are still 8 free slots</pedantic>
> 
> >> and so is the index, and we should be pedantic over not going out of bounds.
> >> We could instead use a flexible array and compute the max opts at run time
> >> as a global. This way the max opts is always updated automatically.
> 
> I don't think it's all that fragile;

We can avoid such compilation warnings.

> this is used only for a global structure
> declaration & initialization.

*Currently used*. I'll later use it to bail out early if the mkfs.xfs.conf stat
file size is too big compared to what we max expect prior letting a library
chew on data. This lets us bail out early instead of having the library stall.
Its a rare corner case but can easily be considered and dealt with.

> If you add too many new members to opt_params,
> compilation will issue warnings:
> 
> xfs_mkfs.c:342: warning: excess elements in array initializer
> xfs_mkfs.c:342: warning: (near initialization for ‘opts’)
> xfs_mkfs.c:424: warning: excess elements in array initializer
> xfs_mkfs.c:424: warning: (near initialization for ‘opts’)
> xfs_mkfs.c:543: warning: excess elements in array initializer
> xfs_mkfs.c:543: warning: (near initialization for ‘opts’)
> ...

Warnings are not treated as failures though AFAICT, and if you fly
by through then yes, its fragile and will crash with out of bound
array access / segfault.

> and it will be quite obvious, at which point you can bump it up before
> you send that patch.  ;)

Mater of taste I guess, but then again I tend to only deal with kernel code and
to me the above is just recipe for disaster.

> Anyway, because the the definition is never used at runtime, there's
> no need to be computing it at runtime, either.

Not currently used at run time. I do plan on using it later at run time but its
for a rather odd corner case to avoid stalls on parsing  a large file when we
know its just not possible to be parsing it.

> > Mmm, that is a good idea and I see no issue with it. But I'm not sure
> > if it is worth of rewriting this patch as we already use MAX_SUBOPTS
> > anyway. Rather I see it as a standalone patch in the next set. What do
> > you think?
> 
> I think that given how it's used, there is no real need for any extra
> complexity.

I find it much simpler, but that seems to be subjective.

> In any case, any changes around this behavior would certainly
> be part of a different patch, because you'd want to be consistent with all
> the other structure initializers, and it would be a functionally separate
> change if it is warranted at all.

The opt_params are separate today though, this patch folds them in together,
what I propose is just to use a flexible array to avoid an explicit size
from the start. Not sure how much more functional of a change that is ?

  Luis