xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfsprogs: cleanup size/log setting flags of mkfs
From: Li Zhong <zhong@xxxxxxxxxxxxxxxxxx>
Date: Mon, 30 Sep 2013 11:04:20 +0800
Cc: Eric Sandeen <sandeen@xxxxxxxxxxx>, xfsprogs <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130929230638.GG26872@dastard>
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>
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. 

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>