xfs
[Top] [All Lists]

Re: [PATCH] xfsprogs: cleanup size/log setting flags of mkfs

To: Li Zhong <zhong@xxxxxxxxxxxxxxxxxx>
Subject: Re: [PATCH] xfsprogs: cleanup size/log setting flags of mkfs
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Sun, 29 Sep 2013 22:14:50 -0500
Cc: Dave Chinner <david@xxxxxxxxxxxxx>, xfsprogs <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1380510260.2985.4.camel@ThinkPad-T5421>
References: <1380272973.2836.5.camel@ThinkPad-T5421> <5245C07A.3000700@xxxxxxxxxxx> <1380437441.3811.9.camel@ThinkPad-T5421> <1380445971.3811.14.camel@ThinkPad-T5421> <20130929230638.GG26872@dastard> <1380510260.2985.4.camel@ThinkPad-T5421>
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:17.0) Gecko/20130801 Thunderbird/17.0.8
On 9/29/13 10:04 PM, Li Zhong wrote:
> On Mon, 2013-09-30 at 09:06 +1000, Dave Chinner wrote:
>> On Sun, Sep 29, 2013 at 05:12:51PM +0800, Li Zhong wrote:
>>> As Eric suggested, we could set both of the size/log flags after we have
>>> parsed the options - and from there on it simply means "manually set".
>>>
>>> After that, we could use just one flag, e.g. *sflag, to check whether 
>>> the corresponding value is manually set or not.
>>
>> It's a start, but I'm not sure that it is an improvement or not.
>> i.e. you're adding yet another piece of logic to the already
>> tortured argument parsing and flag setting.
>>
>> This could be done in the argument parsing itself, without needing
>> separate post-processing code. e.g. changing the parsing code like
>> so:
>>
>>      case N_LOG:
>>              if (!value || *value == '\0')
>>                      reqval('n', nopts, N_LOG);
>> -            if (nlflag)
>> +            if (nlflag > 1)
>>                      respec('n', nopts, N_LOG);
>>              if (nsflag)
>>                      conflict('n', nopts, N_SIZE,
>>                               N_LOG);
>> +            nlflag = 2;
>>              dirblocklog = atoi(value);
>>              if (dirblocklog <= 0)
>>                      illegal(value, "n log");
>> +            nsflag = 1;
>>              dirblocksize = 1 << dirblocklog;
>> -            nlflag = 1;
>>              break;
>>
>> Would acheive exactly the same thing - i.e.
>> a value of 1 means it was initialised, a value of 2 means it was a
>> command line parameter...
>>
>> This means the code checks can be cleaned up as you have done, but
>> we don't need a separate post-processing step for the arguments to
>> set flags that weren't set...
> 
> Thank you for the suggestion, I will try this approach. 
> I think It could also preserve the information which suboption is used
> actually in the command line through the main() function, though it
> seems not needed currently. 

sounds good to me too; just watch out for the conflict/respec stuff to
be sure it all still works... I wasn't super happy with my suggestion
after I made it, I guess. :)

-Eric

> Thanks, Zhong
>>
>>
>>> Signed-off-by: Li Zhong <zhong@xxxxxxxxxxxxxxxxxx>
>>> ---
>>>  mkfs/xfs_mkfs.c | 29 ++++++++++++++++++++++-------
>>>  1 file changed, 22 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>>> index 34bf2ff..aa3f391 100644
>>> --- a/mkfs/xfs_mkfs.c
>>> +++ b/mkfs/xfs_mkfs.c
>>> @@ -1667,11 +1667,26 @@ main(
>>>             dfile = xi.dname;
>>>  
>>>     /*
>>> +    * Later code wants to know if the user manually set a value.
>>> +    * There are two ways to specify on the cmdline; as size or as a log.
>>> +    * if either was used, set both flags - from here on it simply means
>>> +    * "manually set"
>>> +    */
>>> +   if (bsflag || blflag)
>>> +           bsflag = blflag = 1;
>>> +   if (ssflag || slflag)
>>> +           ssflag = slflag = 1;
>>> +   if (isflag || ilflag)
>>> +           isflag = ilflag = 1;
>>> +   if (nsflag || nlflag)
>>> +           nsflag = nlflag = 1;
>>
>> You missed the log sector size/log flags.
>>
>> Cheers,
>>
>> Dave.
> 
> 

<Prev in Thread] Current Thread [Next in Thread>