On 3/24/16 6:15 AM, jtulak@xxxxxxxxxx wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> UPDATE
> o disable finobt when crc=0 no matter if user used -m finobt=X
> o split line > 80 chars
> o remove unused variable
> o add omitted finobtflag
> o change variables in spinodes case to look like surrounding code
> o add I_ALIGN reqval
>
> They are horrible macros that simply obfuscate the code, so
> let's factor the code and make them nice functions.
>
> To do this, add a sb_feat_args structure that carries around the
> variables rather than a strange assortment of variables. This means
> all the default can be clearly defined in a structure
> initialisation, and dependent feature bits are easy to check.
sorry for multiple passes. More comments below.
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> Signed-off-by: Jan Tulak <jtulak@xxxxxxxxxx>
> ---
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 979a860..36bcb9f 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
...
> @@ -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).
> + .spinodes = 0,
> + .log_version = 2,
> + .attr_version = 2,
> + .dir_version = XFS_DFL_DIR_VERSION,
> + .inode_align = XFS_IFLAG_ALIGN,
> + .nci = false,
> + .lazy_sb_counters = true,
> + .projid16bit = false,
> + .crcs_enabled = true,
> + .dirftype = false,
> + .parent_pointers = false,
> + };
>
> platform_uuid_generate(&uuid);
> progname = basename(argv[0]);
...
> @@ -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
...
> + }
> + sb_feat.crcs_enabled = c ? true : false;
> + if (c)
> + sb_feat.dirftype = true;
> break;
> case M_FINOBT:
> if (!value || *value == '\0')
> @@ -1525,8 +1632,8 @@ main(
> c = atoi(value);
> if (c < 0 || c > 1)
> illegal(value, "m finobt");
> - finobt = c;
> - finobtflag = true;
> + sb_feat.finobtflag = true;
yep this should just stay "finobtflag" I think.
> + sb_feat.finobt = c;
> break;
> case M_UUID:
> if (!value || *value == '\0')
...
> @@ -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):
- if (finobt && finobtflag) {
+ if (sb_feat.finobt && sb_feat.finobtflag) {
fprintf(stderr,
_("warning: finobt not supported without CRC support, disabled.\n"));
}
- finobt = 0;
+ sb_feat.finobt = 0;
}
> }
> - finobt = 0;
> }
>
> - if (spinodes && !crcs_enabled) {
> + if (sb_feat.spinodes && !sb_feat.crcs_enabled) {
> fprintf(stderr,
> _("warning: sparse inodes not supported without CRC support, disabled.\n"));
> - spinodes = 0;
> + sb_feat.spinodes = 0;
> }
>
> if (nsflag || nlflag) {
-Eric
|