On Fri, Jul 22, 2016 at 12:40 AM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> 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.
>
OK, thanks for showing the way, I will work on the changes.
In any case, to return to the beginning, the new test added by this
patch is all right.
Thanks,
Jan
--
Jan Tulak
jtulak@xxxxxxxxxx / jan@xxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
