On Thu, Apr 7, 2016 at 2:12 AM, Eric Sandeen <sandeen@xxxxxxxxxxx> wrote:
On 3/24/16 6:15 AM, jtulak@xxxxxxxxxx wrote:
> @@ -1262,10 +1358,11 @@ main(
> switch (getsubopt(&p, (constpp)iopts, &value)) {
> case I_ALIGN:
> if (!value || *value == '\0')
> - value = "1";
> - iaflag = atoi(value);
> - if (iaflag < 0 || iaflag > 1)
> + reqval('i', iopts, I_ALIGN);
> + c = atoi(value);
> + if (c < 0 || c > 1)
> illegal(value, "i align");
> + sb_feat.inode_align = c ? true : false;
> break;
> case I_LOG:
> if (!value || *value == '\0')
Hm, this seems wrong, as well - per the man page:
"If the value is omitted, 1 is assumed."
but this change with the reqval() removes that, doesn't it? Why?
(it's fixed later, but there is no reason to break it mid-series...)
Changed back to default 1. As for the origin of the change, most likely a copy&paste from some other place, where wasn't a default value, or it was there since I took over the patchset.
On Thu, Apr 7, 2016 at 3:43 AM, Eric Sandeen <sandeen@xxxxxxxxxxx> wrote:
> @@ -981,11 +1077,21 @@ main(
> int worst_freelist;
> libxfs_init_t xi;
> struct fs_topology ft;
> - int lazy_sb_counters;
> - int crcs_enabled;
> - int finobt;
> - bool finobtflag;
> - int spinodes;
> + struct sb_feat_args sb_feat = {
> + .finobt = 1,
> + .finobtflag = false,
should we really have "finobtflag" in this structure?
This structure should only carry feature selections, not feature
specification flags I think. Why is this the only such flag
in the structure?
Pretty sure finobtflag should stay a variable for now
just like lvflag (which goes with log_version).
It might be right to move it out, but the flag is removed few patches later entirely. Is it worth of the work? I would say nah, let it die where it is. :-)
...
> @@ -1517,7 +1617,14 @@ main(
> c = atoi(value);
> if (c < 0 || c > 1)
> illegal(value, "m crc");
> - crcs_enabled = c;
> + if (c && nftype) {
> + fprintf(stderr,
> +_("cannot specify both crc and ftype\n"));
> + usage();
hm, why is conflict checking added? It's not what the commit says
the patch does.
It also regresses the bug I fixed in
commit b990de8ba4e2df2bc76a140799d3ddb4a0eac4ce
Author: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Tue Aug 18 17:53:17 2015 +1000
mkfs.xfs: fix ftype-vs-crc option combination testing
with this patch, it is broken again:
# mkfs/mkfs.xfs -m crc=0 -n ftype=1 -dfile,name=fsfile,size=16g
<success>
# mkfs/mkfs.xfs -n ftype=1 -m crc=0 -dfile,name=fsfile,size=16g
cannot specify both crc and ftype
Usage: mkfs.xfs
...
Because the patch is much older than your fix, and at the time it was created, it is possible that there wasn't any such check... I would call it the risk of necromancy. :-)
Anyway, I already fixed this issue in this cycle, and added the the ftype, crc order into a test checking for options sanity. Just I didn't submitted the change yet.
...
> @@ -1879,23 +1988,25 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
> } else {
> /*
> * The kernel doesn't currently support crc=0,finobt=1
> - * filesystems. If crcs are not enabled and the user has
> - * explicitly turned them off then silently turn them off
> - * to avoid an unnecessary warning. If the user explicitly
> - * tried to use crc=0,finobt=1, then issue a warning before
> - * turning them off.
> + * filesystems. If crcs are not enabled and the user has not
> + * explicitly turned finobt on, then silently turn it off to
> + * avoid an unnecessary warning. If the user explicitly tried
> + * to use crc=0,finobt=1, then issue a warning before turning
> + * them off.
> */
> - if (finobt && finobtflag) {
> - fprintf(stderr,
> -_("warning: finobt not supported without CRC support, disabled.\n"));
> + if (sb_feat.finobt){
> + if (sb_feat.finobtflag) {
> + fprintf(stderr,
> + _("warning: finobt not supported without CRC support, disabled.\n"));
> + }
> + sb_feat.finobt = 0;
like I mentioned, just this, I think (assuming we like the silent turning
off, but that would be a different patch):
Merging the conditions is indeed cleaner.
And I will change it to failure, if the conflicting options are given explicitly. Just a small patch adding "usage();" and removing "warning"...
Cheers,
Jan
_______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
