* [PATCH] xfsprogs: make log/size consistent for mkfs's -s option
@ 2013-09-27 9:09 Li Zhong
2013-09-27 17:29 ` Eric Sandeen
0 siblings, 1 reply; 11+ messages in thread
From: Li Zhong @ 2013-09-27 9:09 UTC (permalink / raw)
To: xfsprogs
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.
$ 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) {
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,
--
1.8.1.4
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] xfsprogs: make log/size consistent for mkfs's -s option 2013-09-27 9:09 [PATCH] xfsprogs: make log/size consistent for mkfs's -s option Li Zhong @ 2013-09-27 17:29 ` Eric Sandeen 2013-09-27 23:39 ` Dave Chinner ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Eric Sandeen @ 2013-09-27 17:29 UTC (permalink / raw) To: Li Zhong; +Cc: xfsprogs 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> 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. 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xfsprogs: make log/size consistent for mkfs's -s option 2013-09-27 17:29 ` Eric Sandeen @ 2013-09-27 23:39 ` Dave Chinner 2013-09-29 6:45 ` Li Zhong 2013-09-29 6:50 ` [PATCH v2] " Li Zhong 2 siblings, 0 replies; 11+ messages in thread From: Dave Chinner @ 2013-09-27 23:39 UTC (permalink / raw) To: Eric Sandeen; +Cc: Li Zhong, xfsprogs On Fri, Sep 27, 2013 at 12:29:30PM -0500, Eric Sandeen wrote: > (mkfs.xfs is so crufty :( ) Yes, it needs to go on a massive factoring program.... Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xfsprogs: make log/size consistent for mkfs's -s option 2013-09-27 17:29 ` Eric Sandeen 2013-09-27 23:39 ` Dave Chinner @ 2013-09-29 6:45 ` Li Zhong 2013-09-29 6:50 ` [PATCH v2] " Li Zhong 2 siblings, 0 replies; 11+ messages in thread From: Li Zhong @ 2013-09-29 6:45 UTC (permalink / raw) To: Eric Sandeen; +Cc: xfsprogs 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] xfsprogs: make log/size consistent for mkfs's -s option 2013-09-27 17:29 ` Eric Sandeen 2013-09-27 23:39 ` Dave Chinner 2013-09-29 6:45 ` Li Zhong @ 2013-09-29 6:50 ` Li Zhong 2013-09-29 9:12 ` [PATCH] xfsprogs: cleanup size/log setting flags of mkfs Li Zhong 2 siblings, 1 reply; 11+ messages in thread From: Li Zhong @ 2013-09-29 6:50 UTC (permalink / raw) To: Eric Sandeen; +Cc: xfsprogs 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. $ 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> --- v2: fix an error in v1 found by Eric reviewing the code. 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..34bf2ff 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)) { 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, -- 1.8.1.4 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH] xfsprogs: cleanup size/log setting flags of mkfs 2013-09-29 6:50 ` [PATCH v2] " Li Zhong @ 2013-09-29 9:12 ` Li Zhong 2013-09-29 23:06 ` Dave Chinner 0 siblings, 1 reply; 11+ messages in thread From: Li Zhong @ 2013-09-29 9:12 UTC (permalink / raw) To: Eric Sandeen; +Cc: xfsprogs 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. Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> --- 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; + + /* * Blocksize and sectorsize first, other things depend on them * For RAID4/5/6 we want to align sector size and block size, * so we need to start with the device geometry extraction too. */ - if (!blflag && !bsflag) { + if (!bsflag) { blocklog = XFS_DFL_BLOCKSIZE_LOG; blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG; } @@ -1693,7 +1708,7 @@ main( * ft.sectoralign will never be set. */ sectorsize = blocksize; - } else if (!ssflag && !slflag) { + } else if (!ssflag) { /* * Unless specified manually on the command line use the * advertised sector size of the device. We use the physical @@ -1721,7 +1736,7 @@ _("switching to logical sector size %d\n"), } } - if (ft.sectoralign || (!ssflag && !slflag)) { + if (ft.sectoralign || !ssflag) { sectorlog = libxfs_highbit32(sectorsize); if (loginternal) { lsectorsize = sectorsize; @@ -1731,7 +1746,7 @@ _("switching to logical sector size %d\n"), if (sectorsize < XFS_MIN_SECTORSIZE || sectorsize > XFS_MAX_SECTORSIZE || sectorsize > blocksize) { - if (ssflag || slflag) + if (ssflag) fprintf(stderr, _("illegal sector size %d\n"), sectorsize); else fprintf(stderr, @@ -1760,7 +1775,7 @@ _("block size %d cannot be smaller than logical sector size %d\n"), */ if (crcs_enabled) { /* minimum inode size is 512 bytes, ipflag checked later */ - if ((isflag || ilflag) && inodelog < XFS_DINODE_DFL_CRC_LOG) { + if (isflag && inodelog < XFS_DINODE_DFL_CRC_LOG) { fprintf(stderr, _("Minimum inode size for CRCs is %d bytes\n"), 1 << XFS_DINODE_DFL_CRC_LOG); @@ -1804,7 +1819,7 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n")); } } - if (nsflag || nlflag) { + if (nsflag) { if (dirblocksize < blocksize || dirblocksize > XFS_MAX_BLOCKSIZE) { fprintf(stderr, _("illegal directory block size %d\n"), @@ -1850,7 +1865,7 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n")); if (ipflag) { inodelog = blocklog - libxfs_highbit32(inopblock); isize = 1 << inodelog; - } else if (!ilflag && !isflag) { + } else if (!isflag) { inodelog = crcs_enabled ? XFS_DINODE_DFL_CRC_LOG : XFS_DINODE_DFL_LOG; isize = 1 << inodelog; -- 1.8.1.4 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] xfsprogs: cleanup size/log setting flags of mkfs 2013-09-29 9:12 ` [PATCH] xfsprogs: cleanup size/log setting flags of mkfs Li Zhong @ 2013-09-29 23:06 ` Dave Chinner 2013-09-30 3:04 ` Li Zhong 2013-09-30 5:20 ` [PATCH v2] " Li Zhong 0 siblings, 2 replies; 11+ messages in thread From: Dave Chinner @ 2013-09-29 23:06 UTC (permalink / raw) To: Li Zhong; +Cc: Eric Sandeen, xfsprogs 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... > Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> > --- > 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. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xfsprogs: cleanup size/log setting flags of mkfs 2013-09-29 23:06 ` Dave Chinner @ 2013-09-30 3:04 ` Li Zhong 2013-09-30 3:14 ` Eric Sandeen 2013-09-30 5:20 ` [PATCH v2] " Li Zhong 1 sibling, 1 reply; 11+ messages in thread From: Li Zhong @ 2013-09-30 3:04 UTC (permalink / raw) To: Dave Chinner; +Cc: Eric Sandeen, xfsprogs 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@linux.vnet.ibm.com> > > --- > > 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. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xfsprogs: cleanup size/log setting flags of mkfs 2013-09-30 3:04 ` Li Zhong @ 2013-09-30 3:14 ` Eric Sandeen 2013-09-30 5:24 ` Li Zhong 0 siblings, 1 reply; 11+ messages in thread From: Eric Sandeen @ 2013-09-30 3:14 UTC (permalink / raw) To: Li Zhong; +Cc: xfsprogs 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@linux.vnet.ibm.com> >>> --- >>> 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. > > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xfsprogs: cleanup size/log setting flags of mkfs 2013-09-30 3:14 ` Eric Sandeen @ 2013-09-30 5:24 ` Li Zhong 0 siblings, 0 replies; 11+ messages in thread From: Li Zhong @ 2013-09-30 5:24 UTC (permalink / raw) To: Eric Sandeen; +Cc: xfsprogs On Sun, 2013-09-29 at 22:14 -0500, Eric Sandeen wrote: > 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. :) :) I just sent out the code, hope I didn't make some silly mistakes... Please help to review, though I might only be able to check mail ~ten days later -- we have a long holiday starting from tomorrow :) Thanks, Zhong > > -Eric > > > Thanks, Zhong > >> > >> > >>> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> > >>> --- > >>> 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. > > > > > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] xfsprogs: cleanup size/log setting flags of mkfs 2013-09-29 23:06 ` Dave Chinner 2013-09-30 3:04 ` Li Zhong @ 2013-09-30 5:20 ` Li Zhong 1 sibling, 0 replies; 11+ messages in thread From: Li Zhong @ 2013-09-30 5:20 UTC (permalink / raw) To: Dave Chinner; +Cc: Eric Sandeen, xfsprogs As Dave suggested, for the size/log flags, we could use: a value of 1 to mean it was initialized (by the other suboption), a value of 2 to mean it was a command lien parameter. And after the options are parsed, if we want to check whether a value is set manually or not, we could just use one of the flags, e.g. *sflag: non-zero means it was manually set ( 2 - by size, 1 - by log ), zero means it was not manually set. There are also some small option checking order adjustments, so respec checking is done before conflict checking -- just make them consistent with others. Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> --- mkfs/xfs_mkfs.c | 88 ++++++++++++++++++++++++++++++++------------------------- 1 file changed, 50 insertions(+), 38 deletions(-) diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c index 34bf2ff..b7c1ca8 100644 --- a/mkfs/xfs_mkfs.c +++ b/mkfs/xfs_mkfs.c @@ -1011,7 +1011,7 @@ main( case B_LOG: if (!value || *value == '\0') reqval('b', bopts, B_LOG); - if (blflag) + if (blflag > 1) respec('b', bopts, B_LOG); if (bsflag) conflict('b', bopts, B_SIZE, @@ -1020,12 +1020,13 @@ main( if (blocklog <= 0) illegal(value, "b log"); blocksize = 1 << blocklog; - blflag = 1; + blflag = 2; /* command line parameter */ + bsflag = 1; /* value initialized */ break; case B_SIZE: if (!value || *value == '\0') reqval('b', bopts, B_SIZE); - if (bsflag) + if (bsflag > 1) respec('b', bopts, B_SIZE); if (blflag) conflict('b', bopts, B_LOG, @@ -1036,7 +1037,8 @@ main( !ispow2(blocksize)) illegal(value, "b size"); blocklog = libxfs_highbit32(blocksize); - bsflag = 1; + bsflag = 2; + blflag = 1; break; default: unknown('b', value); @@ -1169,7 +1171,7 @@ main( case D_SECTLOG: if (!value || *value == '\0') reqval('d', dopts, D_SECTLOG); - if (slflag) + if (slflag > 1) respec('d', dopts, D_SECTLOG); if (ssflag) conflict('d', dopts, D_SECTSIZE, @@ -1178,12 +1180,13 @@ main( if (sectorlog <= 0) illegal(value, "d sectlog"); sectorsize = 1 << sectorlog; - slflag = 1; + slflag = 2; + ssflag = 1; break; case D_SECTSIZE: if (!value || *value == '\0') reqval('d', dopts, D_SECTSIZE); - if (ssflag) + if (ssflag > 1) respec('d', dopts, D_SECTSIZE); if (slflag) conflict('d', dopts, D_SECTLOG, @@ -1195,7 +1198,8 @@ main( illegal(value, "d sectsize"); sectorlog = libxfs_highbit32(sectorsize); - ssflag = 1; + ssflag = 2; + slflag = 1; break; case D_RTINHERIT: fsx.fsx_xflags |= \ @@ -1236,19 +1240,20 @@ main( case I_LOG: if (!value || *value == '\0') reqval('i', iopts, I_LOG); - if (ilflag) + if (ilflag > 1) respec('i', iopts, I_LOG); - if (ipflag) - conflict('i', iopts, I_PERBLOCK, - I_LOG); if (isflag) conflict('i', iopts, I_SIZE, I_LOG); + if (ipflag) + conflict('i', iopts, I_PERBLOCK, + I_LOG); inodelog = atoi(value); if (inodelog <= 0) illegal(value, "i log"); isize = 1 << inodelog; - ilflag = 1; + ilflag = 2; + isflag = 1; break; case I_MAXPCT: if (!value || *value == '\0') @@ -1263,11 +1268,11 @@ main( case I_PERBLOCK: if (!value || *value == '\0') reqval('i', iopts, I_PERBLOCK); - if (ilflag) - conflict('i', iopts, I_LOG, - I_PERBLOCK); if (ipflag) respec('i', iopts, I_PERBLOCK); + if (ilflag > 1) + conflict('i', iopts, I_LOG, + I_PERBLOCK); if (isflag) conflict('i', iopts, I_SIZE, I_PERBLOCK); @@ -1281,19 +1286,20 @@ main( case I_SIZE: if (!value || *value == '\0') reqval('i', iopts, I_SIZE); + if (isflag > 1) + respec('i', iopts, I_SIZE); if (ilflag) conflict('i', iopts, I_LOG, I_SIZE); if (ipflag) conflict('i', iopts, I_PERBLOCK, I_SIZE); - if (isflag) - respec('i', iopts, I_SIZE); isize = cvtnum(0, 0, value); if (isize <= 0 || !ispow2(isize)) illegal(value, "i size"); inodelog = libxfs_highbit32(isize); - isflag = 1; + isflag = 2; + ilflag = 1; break; case I_ATTR: if (!value || *value == '\0') @@ -1415,7 +1421,7 @@ main( case L_SECTLOG: if (!value || *value == '\0') reqval('l', lopts, L_SECTLOG); - if (lslflag) + if (lslflag > 1) respec('l', lopts, L_SECTLOG); if (lssflag) conflict('l', lopts, L_SECTSIZE, @@ -1424,12 +1430,13 @@ main( if (lsectorlog <= 0) illegal(value, "l sectlog"); lsectorsize = 1 << lsectorlog; - lslflag = 1; + lslflag = 2; + lssflag = 1; break; case L_SECTSIZE: if (!value || *value == '\0') reqval('l', lopts, L_SECTSIZE); - if (lssflag) + if (lssflag > 1) respec('l', lopts, L_SECTSIZE); if (lslflag) conflict('l', lopts, L_SECTLOG, @@ -1441,7 +1448,8 @@ main( illegal(value, "l sectsize"); lsectorlog = libxfs_highbit32(lsectorsize); - lssflag = 1; + lssflag = 2; + lslflag = 1; break; case L_LAZYSBCNTR: if (!value || *value == '\0') @@ -1490,7 +1498,7 @@ main( 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, @@ -1499,12 +1507,13 @@ main( if (dirblocklog <= 0) illegal(value, "n log"); dirblocksize = 1 << dirblocklog; - nlflag = 1; + nlflag = 2; + nsflag = 1; break; case N_SIZE: if (!value || *value == '\0') reqval('n', nopts, N_SIZE); - if (nsflag) + if (nsflag > 1) respec('n', nopts, N_SIZE); if (nlflag) conflict('n', nopts, N_LOG, @@ -1516,7 +1525,8 @@ main( illegal(value, "n size"); dirblocklog = libxfs_highbit32(dirblocksize); - nsflag = 1; + nsflag = 2; + nlflag = 1; break; case N_VERSION: if (!value || *value == '\0') @@ -1607,7 +1617,7 @@ main( case S_SECTLOG: if (!value || *value == '\0') reqval('s', sopts, S_SECTLOG); - if (slflag || lslflag) + if (slflag > 1 || lslflag > 1) respec('s', sopts, S_SECTLOG); if (ssflag || lssflag) conflict('s', sopts, S_SECTSIZE, @@ -1618,13 +1628,14 @@ main( lsectorlog = sectorlog; sectorsize = 1 << sectorlog; lsectorsize = sectorsize; - lslflag = slflag = 1; + lslflag = slflag = 2; + lssflag = ssflag = 1; break; case S_SIZE: case S_SECTSIZE: if (!value || *value == '\0') reqval('s', sopts, S_SECTSIZE); - if (ssflag || lssflag) + if (ssflag > 1 || lssflag > 1) respec('s', sopts, S_SECTSIZE); if (slflag || lslflag) conflict('s', sopts, S_SECTLOG, @@ -1638,7 +1649,8 @@ main( sectorlog = libxfs_highbit32(sectorsize); lsectorlog = sectorlog; - lssflag = ssflag = 1; + lssflag = ssflag = 2; + lslflag = slflag = 1; break; default: unknown('s', value); @@ -1671,7 +1683,7 @@ main( * For RAID4/5/6 we want to align sector size and block size, * so we need to start with the device geometry extraction too. */ - if (!blflag && !bsflag) { + if (!bsflag) { blocklog = XFS_DFL_BLOCKSIZE_LOG; blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG; } @@ -1693,7 +1705,7 @@ main( * ft.sectoralign will never be set. */ sectorsize = blocksize; - } else if (!ssflag && !slflag) { + } else if (!ssflag) { /* * Unless specified manually on the command line use the * advertised sector size of the device. We use the physical @@ -1721,7 +1733,7 @@ _("switching to logical sector size %d\n"), } } - if (ft.sectoralign || (!ssflag && !slflag)) { + if (ft.sectoralign || !ssflag) { sectorlog = libxfs_highbit32(sectorsize); if (loginternal) { lsectorsize = sectorsize; @@ -1731,7 +1743,7 @@ _("switching to logical sector size %d\n"), if (sectorsize < XFS_MIN_SECTORSIZE || sectorsize > XFS_MAX_SECTORSIZE || sectorsize > blocksize) { - if (ssflag || slflag) + if (ssflag) fprintf(stderr, _("illegal sector size %d\n"), sectorsize); else fprintf(stderr, @@ -1760,7 +1772,7 @@ _("block size %d cannot be smaller than logical sector size %d\n"), */ if (crcs_enabled) { /* minimum inode size is 512 bytes, ipflag checked later */ - if ((isflag || ilflag) && inodelog < XFS_DINODE_DFL_CRC_LOG) { + if (isflag && inodelog < XFS_DINODE_DFL_CRC_LOG) { fprintf(stderr, _("Minimum inode size for CRCs is %d bytes\n"), 1 << XFS_DINODE_DFL_CRC_LOG); @@ -1804,7 +1816,7 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n")); } } - if (nsflag || nlflag) { + if (nsflag) { if (dirblocksize < blocksize || dirblocksize > XFS_MAX_BLOCKSIZE) { fprintf(stderr, _("illegal directory block size %d\n"), @@ -1850,7 +1862,7 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n")); if (ipflag) { inodelog = blocklog - libxfs_highbit32(inopblock); isize = 1 << inodelog; - } else if (!ilflag && !isflag) { + } else if (!isflag) { inodelog = crcs_enabled ? XFS_DINODE_DFL_CRC_LOG : XFS_DINODE_DFL_LOG; isize = 1 << inodelog; -- 1.8.1.4 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-09-30 5:24 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-09-27 9:09 [PATCH] xfsprogs: make log/size consistent for mkfs's -s option Li Zhong 2013-09-27 17:29 ` Eric Sandeen 2013-09-27 23:39 ` Dave Chinner 2013-09-29 6:45 ` Li Zhong 2013-09-29 6:50 ` [PATCH v2] " Li Zhong 2013-09-29 9:12 ` [PATCH] xfsprogs: cleanup size/log setting flags of mkfs Li Zhong 2013-09-29 23:06 ` Dave Chinner 2013-09-30 3:04 ` Li Zhong 2013-09-30 3:14 ` Eric Sandeen 2013-09-30 5:24 ` Li Zhong 2013-09-30 5:20 ` [PATCH v2] " Li Zhong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox