Re: [PATCH 08/12] mkfs: replace variables with opts table: -b,d,s options

[Jan Tulak <jtulak@redhat.com>]

On Tue, Apr 25, 2017 at 7:27 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Sun, Apr 23, 2017 at 08:54:59PM +0200, Jan Tulak wrote:
>> Remove variables that can be replaced with a direct access to the opts table,
>> so we have it all in a single place, acessible from anywhere.
>>
>> In future, we can remove some passing of values forth and back, but now limit
>> the changes to simple replacement without a change in the logic.
>
> What do you mean passing of values here, as an example with bopts ?

Passing it to a function and then retrieving with pointers, e.g. change this

static void
calc_stripe_factors(
        int             dsu,
        int             dsw,
        int             dsectsz,
        int             lsu,
        int             lsectsz,
        uint64_t        *dsunit,
        uint64_t        *dswidth,
        uint64_t        *lsunit)


to this

static void
calc_stripe_factors(struct opt_params * opts) // or no argument at all...

Which btw needs to be updated anyway because it is still using int...

>
>> This is first of multiple similar patches that do the same, but for other
>> options.
>>
>> Signed-off-by: Jan Tulak <jtulak@redhat.com>
>> ---
>>  mkfs/xfs_mkfs.c | 814 ++++++++++++++++++++++++++++++++++----------------------
>>  1 file changed, 503 insertions(+), 311 deletions(-)
>>
>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>> index 362c9b4..6857c30 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -1485,15 +1480,12 @@ main(
>>       int                     argc,
>>       char                    **argv)
>>  {
>> -     uint64_t                agcount;
>>       xfs_agf_t               *agf;
>>       xfs_agi_t               *agi;
>>       xfs_agnumber_t          agno;
>> -     uint64_t                agsize;
>>       xfs_alloc_rec_t         *arec;
>>       struct xfs_btree_block  *block;
>>       bool                    blflag;
>
> Note: blflag
>
>> -     uint64_t                blocklog;
>>       bool                    bsflag;
>
> Note: bsflag
>
>>       memset(&fsx, 0, sizeof(fsx));
>> @@ -1629,6 +1613,7 @@ main(
>>                       break;
>>               case 'b':
>>                       p = optarg;
>> +                     uint64_t tmp;
>>                       while (*p != '\0') {
>>                               char    **subopts =
>>                                               (char **)opts[OPT_B].subopts;
>> @@ -1636,19 +1621,17 @@ main(
>>
>>                               switch (getsubopt(&p, subopts, &value)) {
>>                               case B_LOG:
>> -                                     blocklog = parse_conf_val(OPT_B, B_LOG,
>> -                                                               value);
>> -                                     blocksize = 1 << blocklog;
>> +                                     tmp = parse_conf_val(OPT_B, B_LOG,
>> +                                                          value);
>> +                                     set_conf_val(OPT_B, B_SIZE, 1 << tmp);
>>                                       blflag = 1;
>
> This is a collateral variable set when blog is set. If we have to parse the
> blog again, it means similar code would be needed.
>
>> -                                     set_conf_val(OPT_B, B_SIZE, blocksize);
>>                                       break;
>>                               case B_SIZE:
>> -                                     blocksize = parse_conf_val(OPT_B,
>> -                                                                B_SIZE,
>> -                                                                value);
>> -                                     blocklog = libxfs_highbit32(blocksize);
>> +                                     tmp = parse_conf_val(OPT_B, B_SIZE,
>> +                                                          value);
>> +                                     set_conf_val(OPT_B, B_LOG,
>> +                                             libxfs_highbit32(tmp));
>>                                       bsflag = 1;
>
> Likewise for bsflag.
>
>> -                                     set_conf_val(OPT_B, B_LOG, blocklog);
>>                                       break;
>>                               default:
>>                                       unknown('b', value);
>
> What I'm questioning is whether or not it makes sense instead for parse_conf_val()
> to return void and do all this meddling for us, this would require stuffing any
> collateral variables into a struct and passing that to parse_conf_val and using
> it on main() as well.

Again, this (both logic and flags) is something I want to do later.
And in fact, I think that perhaps the whole manual loop in main() can
be removed in the future once the case logic is moved elsewhere.
Because then we can extract specific cases from the opts table with
the help of some helper functions...

Jan

>
>   Luis



-- 
Jan Tulak
jtulak@redhat.com / jan@tulak.me