On 4/25/17 2:45 AM, Jan Tulak wrote:
> On Tue, Apr 25, 2017 at 5:04 AM, Luis R. Rodriguez <mcgrof@xxxxxxxxxx> 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@xxxxxxxxxx>
>>> ---
>>> 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; this is used only for a global structure
declaration & initialization. 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’)
...
and it will be quite obvious, at which point you can bump it up before
you send that patch. ;)
Anyway, because the the definition is never used at runtime, there's
no need to be computing it at runtime, either.
> 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. 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.
Thanks,
-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
