Re: [RFC PATCH 00/22] mkfs.xfs: Make stronger conflict checks

[Bill O'Donnell <billodo@redhat.com>]

On Wed, Dec 07, 2016 at 02:27:07PM +0100, Jan Tulak wrote:
> Hi guys,
> 
> this set is a follow-up of some old discussions and further attempts to untangle
> the spaghetti in options parsing. In short, this patchset allows to define
> cross-option conflicts and makes the conflicts detection more robust.

concept seems good. I'll review the patches.

> 
> Until now, we had the ability to define conflicts within one option (e.g. -d
> sunit/su), but things like -i attr=1 -m crc=1 conflict had to be watched for on
> case by case basis somewhere in the code. Now, when even those situations are
> handled by the same code, it is enough to just add a new entry into a table of
> options. Thus, a reduced chance for an error and easier adding of new cases.
> 
> One of the biggest changes in this set is that user input is now stored in
> directly in the opts table defining allowed range and the like, and variables
> in the main() of mkfs.xfs are now just aliases/pointers. This allows as to do
> conditional checks based on actual values, not only on occurence of an option.
> 
> (A technical note here is that not every value can be saved in a single place
> like this. Some values are already stored in a table or structure and I wanted
> to avoid modifying anything outside of xfs_mkfs.c.)
> 
> I tested it with full xfstests suit and the only failed tests I saw are because
> some ambiguity in arguments parsing was removed. E.g. sometimes it was possible
> to specify size in blocks without stating the blocksize first, even if manpage
> explicitly requires -b or -s to be used before.
> 
> I already submitted part of this patchset as RFC before, but as I got no reply,
> I tried to finish it before submitting again. So, this set works as it is. I
> still have some questions, but they can be answered with "let's keep it as it
> is."
> 
> Number one is simple: What values can use block/sector sizes as user input?
> There is an inconsistency or ambiguity between manual page and the code. Look
> at man page for -d agsize.
> 
> 	agsize=value
> 		This is an alternative to using the  agcount  subop‐
> 		tion.  The  value is the desired size of the alloca‐
> 		tion group expressed in bytes (usually using  the  m
> 		                     ^^^^^^^^
> 		or  g  suffixes).   This value must be a multiple of
> 		[ ... ]
> 
> 
> The option -d agsize explicitly states that it accepts size in bytes, in a
> similar tone to the one used for describe allowed values for -s/-b size:
> 
> 	value in bytes with size=value
> 	      ^^^^^^^^
> 
> However, -d agsize=1234s input was accepted as valid until now. Is the manual
> page misleading, or are the options where b/s suffix is forbidden are
> block/sector size definitions? I decided to err on the compatibility side and
> kept the current behaviour - only blocksize or sectorsize can't be stated in
> blocks and sectors, but it can be easily changed.
> 
> I will send an update for xfstests once I know what behaviour is correct.
> 
> 
> The other question about this patchset is: As we are saving all the values in
> the opt_params table, and the values have different types, I thought it
> necessary to not use a single data type for everything and created an union
> field (could be easily changed to struct, that would not change anything
> important). Do you see any non-adressed issue with this approach? Is there
> another way how to solve the problem?
> 
> If nothing else, numbers and strings can't be easily saved in a single
> variable. Also, as we are using shift operations, any type conversions
> (like storing everything in long long type) could cause trouble. This is one of
> the reasons why I'm changed the variables in main() to pointers. This allows
> for simple and easy access to the correct union field, so unless one is adding
> a new option, there should be no need to remember the correct date type. If the
> pointer assignment is done correctly, then GCC will watch for type mismatch.
> 
> I really couldn't find out better solution, but see for yourself, this change
> is done in "mkfs: Change all value fields in opt structures into unions"
> and "mkfs: use old variables as pointers to the new opts struct values".
> 
> 
> So, I think this is all I wanted to cover in the cover letter. :-)
> I will be glad for any comments or bugs you find out.
> 
> Thanks for your time,
> 
> Jan
> 
> 
> Jan Tulak (22):
>   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: change conflict checks to utilize the new conflict structure
>   mkfs: change when to mark an option as seen
>   mkfs: add test_default_value into conflict struct
>   mkfs: expand conflicts declarations to named declaration
>   mkfs: remove zeroed items from conflicts declaration
>   mkfs: rename defaultval to flagval in opts
>   mkfs: replace SUBOPT_NEEDS_VAL for a flag
>   mkfs: Change all value fields in opt structures into unions
>   mkfs: use old variables as pointers to the new opts struct values
>   mkfs: prevent sector/blocksize to be specified as a number of blocks
>   mkfs: subopt flags should be saved as bool
>   mkfs: move uuid empty string test to getstr()
>   mkfs: remove duplicit checks
>   mkfs: prevent multiple specifications of a single option
> 
>  mkfs/xfs_mkfs.c | 2952 +++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 1864 insertions(+), 1088 deletions(-)
> 
> -- 
> 2.8.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html