Re: [PATCH] xfsprogs: make log/size consistent for mkfs's -s option

[Li Zhong <zhong@linux.vnet.ibm.com>]

On Fri, 2013-09-27 at 12:29 -0500, Eric Sandeen wrote:
> On 9/27/13 4:09 AM, Li Zhong wrote:
> > It seems using -s log is not able to set the sectsz correctly. Because slflag
> > is set but ignored by later codes, so the advertised sector size of the device
> > is used instead.
> 
> (below is just musing about this in general, skip down to the patch for one
> flaw, I think).
> 
> One wonders why it was originally written to accept both, in the first place :(
> 
> And looking at git history, checking only ssflag later was my mistake.  :(
> 
> by the time we're done with getopt, we've got both sectorsize and sectorlog
> set anyway, and we know if it was specified on the commandline.  Maybe we should
> just set them both right after getopt, like:
> 
>         /*
>          * 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 (ssflag || slflag)
>                 ssflag = slflag = 1;
>         <etc for other flags>

I think it is better, after this is done, later code could use one
*sflag to check whether the value is manually set. I will give it a try.

> 
> Anyway, other than one problem below, I think this is ok to solve this
> particular problem.  The others, at least nsflag/nlflag, isflag/ilflag,
> and bsflag/blflag all look ok.
> 
> I just wonder if we need to re-think how this is handled in general,
> so for all of the various (size=|log=) type options, we don't have to
> keep remembering to check both flags.
> 
> (mkfs.xfs is so crufty :(  )
> 
> > $ mkfs.xfs -f -s size=4096 /dev/sdd
> > meta-data=/dev/sdd               isize=256    agcount=2, agsize=4096 blks
> >          =                       sectsz=4096  attr=2, projid32bit=1
> > 	 ......
> > 
> > $ mkfs.xfs -f -s log=12 /dev/sdd
> > meta-data=/dev/sdd               isize=256    agcount=2, agsize=4096 blks
> >          =                       sectsz=2048  attr=2, projid32bit=1
> > 	 ......
> > 
> > Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
> > ---
> >  mkfs/xfs_mkfs.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index eafbed3..9243044 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -1693,7 +1693,7 @@ main(
> >  		 * ft.sectoralign will never be set.
> >  		 */
> >  		sectorsize = blocksize;
> > -	} else if (!ssflag) {
> > +	} else if (!ssflag && !slflag) {
> >  		/*
> >  		 * Unless specified manually on the command line use the
> >  		 * advertised sector size of the device.  We use the physical
> > @@ -1721,7 +1721,7 @@ _("switching to logical sector size %d\n"),
> >  		}
> >  	}
> >  
> > -	if (ft.sectoralign || !ssflag) {
> > +	if (ft.sectoralign || !ssflag || !slflag) {
> 
> Shouldn't this be:
> 
> if (ft.sectoralign || (!ssflag && !slflag)) {
> 
> ?  Because today only one or the other can be set;  !ssflag || !slflag will always be true I think.

Sorry, it's my mistake... I'll update it and give a v2. 

Thanks, Zhong

> 
> Thanks,  
> -Eric
> 
> >  		sectorlog = libxfs_highbit32(sectorsize);
> >  		if (loginternal) {
> >  			lsectorsize = sectorsize;
> > @@ -1731,7 +1731,7 @@ _("switching to logical sector size %d\n"),
> >  
> >  	if (sectorsize < XFS_MIN_SECTORSIZE ||
> >  	    sectorsize > XFS_MAX_SECTORSIZE || sectorsize > blocksize) {
> > -		if (ssflag)
> > +		if (ssflag || slflag)
> >  			fprintf(stderr, _("illegal sector size %d\n"), sectorsize);
> >  		else
> >  			fprintf(stderr,
> > 
> 


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs