Re: [PATCH v4 01/53] xen: add an optional string end parameter to parse_bool()

[Juergen Gross <jgross@suse.com>]

On 24/08/17 17:43, Andrew Cooper wrote:
> On 23/08/17 18:33, Juergen Gross wrote:
>> Add a parameter to parse_bool() to specify the end of the to be
>> parsed string. Specifying it as NULL will preserve the current
>> behavior to parse until the end of the input string, while passing
>> a non-NULL pointer will specify the first character after the input
>> string.
>>
>> This will allow to parse boolean sub-strings without having to
>> write a NUL byte into the input string.
>>
>> Modify all users of parse_bool() to pass NULL for the new parameter.
> 
> So I already had plans for parse_bool() during the XSA-226 embaro
> period, but couldn't discuss any of them, and this series appeared in
> the meantime.
> 
> One rather confusing problem we have is that top level booleans behave
> differently to sub-booleans.
> 
> Top-level booleans support all kinds of ={0,true,yes, ...} qualifiers,
> as well as no- prefixes.  sub-booleans may or may not support the
> qualifiers, and where they do support the no- prefixes, the same prefix
> gets silently eaten for non-boolean suboptions.

This is the "iommu=" case, right? And in fact the same statement is true
for all top-level parameters: you can easily specify "no-dma_bits=..."
which will be accepted. Only top-level custom parameters are handled in
a sane way with the "no-" prefix (they are accepted only with no option
value), and boolean parameters, of course.

> I had planned to modify parse_bool() to be
> 
> int parse_bool(const char *field, const char *s)
> {
>     ...
> }
> 
> which cases care of considering the "no-" prefix, optionally skips the
> field name if it matches exactly, and then performs the current logic on
> the remainder of the string.  This way, boolean options should work
> consistently wherever they are.
> 
> It also means that a lot of custom_params() need simplifying to always
> pass intended boolean options to parse_bool().

I believe they do so in most cases.

> Could we see about merging this work together, rather than having two
> series going and modifying how the parsing works?

Hmm, I'm not sure it is worth the effort. Doing a quick search I found
only the iommu case where this would be relevant. All other cases are
handled correctly by _cmdline_parse(): a custom parameter prefixed with
"no-" is accepted only without a value specification.

I'd rather add one further patch to my series to correct the iommu case
and another one to fix _cmdline_parse() for the other cases where the
"no-" prefix is accepted for non-boolean parameters.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel