[RFC PATCH 0/8] mkfs: centralised conflict detection

* [RFC PATCH 0/8] mkfs: centralised conflict detection
@ 2016-08-02 15:42 Jan Tulak
  2016-08-02 15:42 ` [PATCH 1/8] mkfs: remove intermediate getstr followed by getnum Jan Tulak
                   ` (7 more replies)
  0 siblings, 8 replies; 9+ messages in thread
From: Jan Tulak @ 2016-08-02 15:42 UTC (permalink / raw)
  To: xfs; +Cc: Jan Tulak

Hi guys

This is set follows the earlier cleaning of mkfs by utilising the new code to
do also inter-option conflicts checks, and is inspired by Dave's idea in
"xfstests: Add mkfs input validation tests" thread.

With these patches, adding -i attr=1 -m crc=1  conflict is as simple as:
                        { .index = I_ATTR,
-                         .conflicts = { {LAST_CONFLICT} },
+                         .conflicts = { {OPT_M, M_CRC, true, 1, 1,
+               "V2 attribute format always enabled on CRC enabled filesytems."},
+                                        {LAST_CONFLICT} },

And a similar change in the corresponding option. You can read it as:
I_ATTR has a conflict with M_CRC depending on input values (the "true"), when
M_CRC is 1 (the first 1) and I_ATTR is 1 (the second 1). Report it with
message... (I should probably rewrite it to use named initialization, instead
of positional.)

This change  should remove some other code from mkfs and make conflicts
consistent.  The patch set, as it is, should be mostly done. I think there are
still some conflicts to be converted, and some new lines added/removed where
they are not needed,  but the main reason why this is RFC is an issue with
defaults.

Detection of conflicts in user input is working like a charm. However, if some
option is conflicting with default settings, I have a problem. For example,
lets look on -m crc=1 -n ftype=0. crc=1 is the default, so if the user inputs
-n ftype=0 alone, it is still a conflict and the old code detects it once it
loads and parses all user input. The new does not.

The conflicts detecting code added in previous mkfs cleaning, which this
patchset is extending, does conflicts detection at the moment it sees an
option/flag, during getopt loop. Which means I can't just raise a red flag
when something is in conflict with default, because it is possible that the
very next option will change that default.

I left the old code here and only added a comment (it is in the last patch),
but there are other options with the same issue...

The solutions I came up are here, but I would like to know some other people's
thoughts before going for any of them. They are sorted by the amount of
necessary changes.

1) Leave the old code there. This brings some duplicity, but not every option
has this issue, so I think that overall it would be still a gain.

2) Do minimal changes to move the conflict checks after the getopt loop. I
think that this should be possible without too many changes - I would remove
all conflict-related things from getnum/getstr and instead would add a loop to
go through the entire table with options after the getopt loop. So - one loop
to fill the table, with user-given values, second loop to check for conflicts.

3) Rewrite conflicts from scratch and check them once everything was parsed and
all options loaded. Similar to 2, but rather than making minimal changes, I
would take the conflicts out of the current opt_params table and made a second
table for conflicts only. This would also allow for merging the two entries
necessary for every conflict (one from each side) into a single one.

My thoughts about these ideas: 1 is clumsy and doesn't look correct, so I think
about 2 and 3. For now, 2 would be easier, but I think also about adding range
conflicts (inode size with and without crc, ...) and possibly other things and
then I'm not sure which implementation would be better.

I hope I described it well enough so you don't have to study every line of the
code... Anyway, look on this patchset and tell me your thoughts.

Thank you :-)
Jan

Jan Tulak (8):
  mkfs: remove intermediate getstr followed by getnum
  mkfs: merge tables for opts parsing into one table
  mkfs: extend opt_params with a value field
  mkfs: change conflicts array into a table capable of cross-option
    addressing
  mkfs: add a check for conflicting values
  mkfs: add cross-section conflict checks
  mkfs: Move opts related #define to one place
  mkfs: move conflicts into the table

 mkfs/xfs_mkfs.c | 1647 +++++++++++++++++++++++++++++++------------------------
 1 file changed, 938 insertions(+), 709 deletions(-)

-- 
2.5.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 9+ messages in thread