On Thu, Jul 21, 2016 at 04:24:52PM +0200, Jan Tulak wrote:
> On Thu, Jul 21, 2016 at 1:54 AM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> >> >> I think you need to test on a 4k sector size disk. I use scsi_debug to
> >> >> simulate physical 4k sector disk to reproduce this:
> >> >>
> >> >> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe -r scsi_debug
> >> >> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe scsi_debug dev_size_mb=128 physblk_exp=3
> >> >> [root@dhcp-66-86-11 xfsprogs-dev]# blockdev --getbsz --getpbsz --getss /dev/sdc
> >> >> 4096
> >> >> 4096
> >> >> 512
> >> >> [root@dhcp-66-86-11 xfsprogs-dev]# mkfs -t xfs -l version=1 -m crc=1 /dev/sdc
> >
> > So this is an invalid filesystem configuration. It should be
> > detected as such during command line parsing and rejected before we
> > get anywhere near checking topology constraints. In mkfs
> > terms, it's a conflicting option set.
> >
> >> And the culprit is in mkfs, some forty lines before the crc & log version check:
> >>
> >> 2026 ⇥ } else if (lsectorsize > XFS_MIN_SECTORSIZE && !lsu && !lsunit) {
> >> 2027 ⇥ ⇥ lsu = blocksize;
> >> 2028 ⇥ ⇥ sb_feat.log_version = 2;
> >> 2029 ⇥ }
> >>
> >> The possible solutions I can think of are:
> >
> > None of which really appeal because, IMO, they are trying to solve
> > the wrong problem.
> >
> > The whole point of moving to table based command line option parsing
> > is that we can encode these sorts of conflicts into the option
> > table. The conflict resolution in the option table is currently not
> > complete - it can only encode and detect conflicts within a
> > suboption type, but not across suboption types (e.g. within -d
> > suboptions, but not between -d and -l suboptions).
> >
> > This is simply because I never got as far as implementing this level
> > of conflict encoding/resolution. In essence, the conflict array
> > needs to define the sub option type, the suboption that is
> > in conflict and the value that it conflicts against. Hence the
> > conflicts table can then encode such things as "version 1 logs are
> > invalid for CRC enabled filesystems" and vice versa.
> >
>
> Ok, in long term, the correct way is to extend the conflicts table.
Not long term. It's fairly simple to do.
1. Convert all the individual subopt parameter tables to an array of tables
with a defined index for each set of subopts,
2. add a value field to the parameter, and store the CLI value in
it when it is set
3. make the conflicts array in each subopts a structure like:
struct conflicts {
int subopt;
int index;
int invalid_value;
};
and convert all the existing conflicts to this format
4. Define cross-subopt conflicts like this:
.subopt_params = {
{ .index = M_CRC,
- .conflicts = { LAST_CONFLICT },
+ .conflicts = { { LOG, L_VERSION, 1 },
+ { LAST_CONFLICT, 0, 0 }, },
.minval = 0,
.maxval = 1,
.defaultval = 1,
},
And the L_VERSION subopt parameter will have a conflict like
+ .conflicts = { { META, M_CRC, 1 },
+ { LAST_CONFLICT, 0, 0 }, },
5. update the conflict lookup to do cross option lookups via
checking the relevant option conflict table. e.g by checking the
conflict[i].value against subopt[LOG].subopt_params[L_VERSION].value...
> But what in the meantime? Are we going to let it be now until it is
> fixed by the enhanced table?
IMO: fix it once, fix it properly.
> And regarding my question at the end of the mail, I interpret your
> answer as "if the arguments are wrong, fail ASAP and don't try to fix
> it."
The first step in any program shoul dbe to validate user supplied
inputs. Once they are validated and known good, you don't have to
add random code to handle invalid combinations - you can just assume
the inputs are valid to begin with and those corner cases don't need
to be handled.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
