On Tue, Apr 25, 2017 at 7:27 AM, Luis R. Rodriguez <mcgrof@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx / jan@xxxxxxxx
--
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
