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

2017-08-25 Thread Jan Beulich 
>>> On 25.08.17 at 10:31,  wrote:
> On 24/08/17 17:43, Andrew Cooper wrote:
>> 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.

Note how Andrew did say "sub-option": Just grep for "no-" (including
the quotes) and you'll find a few more examples.

I'm not, however, convinced that this really should be done in this
series, as the goal is quite a different one. Yes, this would mean
touching parse_bool() callers a second time, but that's the way
things work.

Jan


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

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

2017-08-25 Thread Juergen Gross 
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

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

2017-08-24 Thread Juergen Gross 
On 24/08/17 17:33, Jan Beulich wrote:
 On 23.08.17 at 19:33,  wrote:
>> @@ -163,20 +163,24 @@ void __init cmdline_parse(const char *cmdline)
>>  #endif
>>  }
>>  
>> -int __init parse_bool(const char *s)
>> +int __init parse_bool(const char *s, const char *e)
>>  {
>> -if ( !strcmp("no", s) ||
>> - !strcmp("off", s) ||
>> - !strcmp("false", s) ||
>> - !strcmp("disable", s) ||
>> - !strcmp("0", s) )
>> +unsigned int len;
>> +
>> +len = e ? e - s : strlen(s);
> 
> If you don't mind, I'd like to see ASSERT(e >= s) added to the middle
> part here; should be easily doable while committing.

Sure, NP.


Juergen


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

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

2017-08-24 Thread Andrew Cooper 
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.

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().

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

~Andrew

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

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

2017-08-24 Thread Jan Beulich 
>>> On 23.08.17 at 19:33,  wrote:
> @@ -163,20 +163,24 @@ void __init cmdline_parse(const char *cmdline)
>  #endif
>  }
>  
> -int __init parse_bool(const char *s)
> +int __init parse_bool(const char *s, const char *e)
>  {
> -if ( !strcmp("no", s) ||
> - !strcmp("off", s) ||
> - !strcmp("false", s) ||
> - !strcmp("disable", s) ||
> - !strcmp("0", s) )
> +unsigned int len;
> +
> +len = e ? e - s : strlen(s);

If you don't mind, I'd like to see ASSERT(e >= s) added to the middle
part here; should be easily doable while committing.

Jan


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

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

2017-08-24 Thread Wei Liu 
On Wed, Aug 23, 2017 at 07:33:54PM +0200, 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.

Reviewed-by: Wei Liu 

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

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

2017-08-24 Thread Tian, Kevin 
> From: Juergen Gross [mailto:jgr...@suse.com]
> Sent: Thursday, August 24, 2017 1:34 AM
> 
> 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.
> 
> Cc: Stefano Stabellini 
> Cc: Julien Grall 
> Cc: Andrew Cooper 
> Cc: George Dunlap 
> Cc: Ian Jackson 
> Cc: Jan Beulich 
> Cc: Konrad Rzeszutek Wilk 
> Cc: Tim Deegan 
> Cc: Wei Liu 
> Cc: Kevin Tian 
> Signed-off-by: Juergen Gross 

Reviewed-by: Kevin Tian 

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

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

2017-08-23 Thread Juergen Gross 
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.

Cc: Stefano Stabellini 
Cc: Julien Grall 
Cc: Andrew Cooper 
Cc: George Dunlap 
Cc: Ian Jackson 
Cc: Jan Beulich 
Cc: Konrad Rzeszutek Wilk 
Cc: Tim Deegan 
Cc: Wei Liu 
Cc: Kevin Tian 
Signed-off-by: Juergen Gross 
---
 xen/arch/arm/acpi/boot.c  |  2 +-
 xen/arch/x86/cpu/vpmu.c   |  2 +-
 xen/arch/x86/mm.c |  2 +-
 xen/arch/x86/nmi.c|  2 +-
 xen/arch/x86/psr.c|  2 +-
 xen/arch/x86/setup.c  |  6 +++---
 xen/arch/x86/x86_64/mmconfig-shared.c |  2 +-
 xen/common/kernel.c   | 28 
 xen/drivers/char/console.c|  2 +-
 xen/drivers/cpufreq/cpufreq.c |  2 +-
 xen/drivers/passthrough/iommu.c   |  2 +-
 xen/drivers/passthrough/vtd/quirks.c  |  2 +-
 xen/include/xen/lib.h |  2 +-
 13 files changed, 30 insertions(+), 26 deletions(-)

diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
index 889208a0ea..a5a6f55f0e 100644
--- a/xen/arch/arm/acpi/boot.c
+++ b/xen/arch/arm/acpi/boot.c
@@ -199,7 +199,7 @@ static void __init parse_acpi_param(char *arg)
 return;
 
 /* Interpret the parameter for use within Xen. */
-if ( !parse_bool(arg) )
+if ( !parse_bool(arg, NULL) )
 param_acpi_off = true;
 else if ( !strcmp(arg, "force") ) /* force ACPI to be enabled */
 param_acpi_force = true;
diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
index 90954ca884..1c0ea10777 100644
--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -80,7 +80,7 @@ static void __init parse_vpmu_params(char *s)
 {
 char *sep, *p = s;
 
-switch ( parse_bool(s) )
+switch ( parse_bool(s, NULL) )
 {
 case 0:
 break;
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index ed77270586..5b0e55d7d9 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -184,7 +184,7 @@ static void __init parse_mmio_relax(const char *s)
 if ( !*s )
 opt_mmio_relax = 1;
 else
-opt_mmio_relax = parse_bool(s);
+opt_mmio_relax = parse_bool(s, NULL);
 if ( opt_mmio_relax < 0 && strcmp(s, "all") )
 opt_mmio_relax = 0;
 }
diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
index 8914581f66..e44f88045e 100644
--- a/xen/arch/x86/nmi.c
+++ b/xen/arch/x86/nmi.c
@@ -54,7 +54,7 @@ static void __init parse_watchdog(char *s)
 return;
 }
 
-switch ( parse_bool(s) )
+switch ( parse_bool(s, NULL) )
 {
 case 0:
 opt_watchdog = false;
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index c2036cbed4..25a85b65b2 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -427,7 +427,7 @@ static void __init parse_psr_bool(char *s, char *value, 
char *feature,
 opt_psr |= mask;
 else
 {
-int val_int = parse_bool(value);
+int val_int = parse_bool(value, NULL);
 
 if ( val_int == 0 )
 opt_psr &= ~mask;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index db5df6956d..414681d5a1 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -110,7 +110,7 @@ static void __init parse_smep_param(char *s)
 return;
 }
 
-switch ( parse_bool(s) )
+switch ( parse_bool(s, NULL) )
 {
 case 0:
 opt_smep = 0;
@@ -136,7 +136,7 @@ static void __init parse_smap_param(char *s)
 return;
 }
 
-switch ( parse_bool(s) )
+switch ( parse_bool(s, NULL) )
 {
 case 0:
 opt_smap = 0;
@@ -160,7 +160,7 @@ static void __init parse_acpi_param(char *s)
 safe_strcpy(acpi_param, s);
 
 /* Interpret the parameter for use within Xen. */
-if ( !parse_bool(s) )
+if ( !parse_bool(s, NULL) )
 {
 disable_acpi();
 }
diff --git a/xen/arch/x86/x86_64/mmconfig-shared.c 
b/xen/arch/x86/x86_64/mmconfig-shared.c
index 488470bfeb..dbf9ff07fa 100644
--- a/xen/arch/x86/x86_64/mmconfig-shared.c
+++ b/xen/arch/x86/x86_64/mmconfig-shared.c
@@ -37,7 +37,7 @@ static void __init parse_mmcfg(char *s)
 if ( ss )
 *ss = '\0';
 
-if ( !parse_bool(s) )
+if ( !parse_bool(s, NULL) )
 pci_probe &= ~PCI_PROBE_MMCONF;
 else if ( !strcmp(s, "amd_fam10") || !strcmp(s, "amd-fam10") )