| Message ID | 20170823173446.24801-2-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> From: Juergen Gross [mailto:jgross@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 <sstabellini@kernel.org> > Cc: Julien Grall <julien.grall@arm.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: George Dunlap <George.Dunlap@eu.citrix.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: Tim Deegan <tim@xen.org> > Cc: Wei Liu <wei.liu2@citrix.com> > Cc: Kevin Tian <kevin.tian@intel.com> > Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
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 <wei.liu2@citrix.com>
>>> On 23.08.17 at 19:33, <jgross@suse.com> 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
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
On 24/08/17 17:33, Jan Beulich wrote: >>>> On 23.08.17 at 19:33, <jgross@suse.com> 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
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
>>> On 25.08.17 at 10:31, <jgross@suse.com> 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
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") ) pci_probe |= PCI_CHECK_ENABLE_AMD_MMCONF; diff --git a/xen/common/kernel.c b/xen/common/kernel.c index ce7cb8adb5..4979e1c49b 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -114,7 +114,7 @@ static void __init _cmdline_parse(const char *cmdline) simple_strtoll(optval, NULL, 0)); break; case OPT_BOOL: - if ( !parse_bool(optval) ) + if ( !parse_bool(optval, NULL) ) bool_assert = !bool_assert; assign_integer_param(param, bool_assert); break; @@ -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 ( !strncmp("no", s, len) || + !strncmp("off", s, len) || + !strncmp("false", s, len) || + !strncmp("disable", s, len) || + !strncmp("0", s, len) ) return 0; - if ( !strcmp("yes", s) || - !strcmp("on", s) || - !strcmp("true", s) || - !strcmp("enable", s) || - !strcmp("1", s) ) + if ( !strncmp("yes", s, len) || + !strncmp("on", s, len) || + !strncmp("true", s, len) || + !strncmp("enable", s, len) || + !strncmp("1", s, len) ) return 1; return -1; diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index f0659fba1b..8f2a24496a 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -605,7 +605,7 @@ static int printk_prefix_check(char *p, char **pp) static void __init parse_console_timestamps(char *s) { - switch ( parse_bool(s) ) + switch ( parse_bool(s, NULL) ) { case 0: opt_con_timestamp_mode = TSM_NONE; diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c index fd82ef5dce..5580bd370d 100644 --- a/xen/drivers/cpufreq/cpufreq.c +++ b/xen/drivers/cpufreq/cpufreq.c @@ -69,7 +69,7 @@ static void __init setup_cpufreq_option(char *str) if ( arg ) *arg++ = '\0'; - choice = parse_bool(str); + choice = parse_bool(str, NULL); if ( choice < 0 && !strcmp(str, "dom0-kernel") ) { diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index 5e81813942..f1aefc47ce 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -92,7 +92,7 @@ static void __init parse_iommu_param(char *s) if ( ss ) *ss = '\0'; - if ( !parse_bool(s) ) + if ( !parse_bool(s, NULL) ) iommu_enable = 0; else if ( !strcmp(s, "force") || !strcmp(s, "required") ) force_iommu = val; diff --git a/xen/drivers/passthrough/vtd/quirks.c b/xen/drivers/passthrough/vtd/quirks.c index 5bbbd96d51..d6dd671dbf 100644 --- a/xen/drivers/passthrough/vtd/quirks.c +++ b/xen/drivers/passthrough/vtd/quirks.c @@ -251,7 +251,7 @@ static void __init parse_snb_timeout(const char *s) { int t; - t = parse_bool(s); + t = parse_bool(s, NULL); if ( t < 0 ) { if ( *s == '\0' ) diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h index 995a85a7db..8e57bbd021 100644 --- a/xen/include/xen/lib.h +++ b/xen/include/xen/lib.h @@ -71,7 +71,7 @@ struct domain; void cmdline_parse(const char *cmdline); -int parse_bool(const char *s); +int parse_bool(const char *s, const char *e); /*#define DEBUG_TRACE_DUMP*/ #ifdef DEBUG_TRACE_DUMP
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 <sstabellini@kernel.org> Cc: Julien Grall <julien.grall@arm.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Tim Deegan <tim@xen.org> Cc: Wei Liu <wei.liu2@citrix.com> Cc: Kevin Tian <kevin.tian@intel.com> Signed-off-by: Juergen Gross <jgross@suse.com> --- 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(-)