On 27.03.2023 12:59, Luca Fancellu wrote:
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -59,6 +59,11 @@ static int __init parse_dom0_mem(const char *s)
> }
> custom_param("dom0_mem", parse_dom0_mem);
>
> +int __init parse_arch_dom0_param(const char *str_begin, const char *str_end)
> +{
> + return -1;
> +}
Please also use -E... here, like x86 does.
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -266,42 +266,30 @@ bool __initdata opt_dom0_pvh = !IS_ENABLED(CONFIG_PV);
> bool __initdata opt_dom0_verbose = IS_ENABLED(CONFIG_VERBOSE_DEBUG);
> bool __initdata opt_dom0_msr_relaxed;
>
> -static int __init cf_check parse_dom0_param(const char *s)
> +int __init parse_arch_dom0_param(const char *str_begin, const char *str_end)
Is there any reason you couldn't stick to the original variable names (s
and ss) or use other meaningful by shorter names like s and e or str and
end (my personal preference among the three in the order given)? That'll
help with line length and hence readability.
> {
> - const char *ss;
> - int rc = 0;
> -
> - do {
> - int val;
> -
> - ss = strchr(s, ',');
> - if ( !ss )
> - ss = strchr(s, '\0');
> + int val, rc = 0;
>
> - if ( IS_ENABLED(CONFIG_PV) && !cmdline_strcmp(s, "pv") )
> - opt_dom0_pvh = false;
> - else if ( IS_ENABLED(CONFIG_HVM) && !cmdline_strcmp(s, "pvh") )
> - opt_dom0_pvh = true;
> + if ( IS_ENABLED(CONFIG_PV) && !cmdline_strcmp(str_begin, "pv") )
> + opt_dom0_pvh = false;
> + else if ( IS_ENABLED(CONFIG_HVM) && !cmdline_strcmp(str_begin, "pvh") )
> + opt_dom0_pvh = true;
> #ifdef CONFIG_SHADOW_PAGING
> - else if ( (val = parse_boolean("shadow", s, ss)) >= 0 )
> - opt_dom0_shadow = val;
> + else if ( (val = parse_boolean("shadow", str_begin, str_end)) >= 0 )
> + opt_dom0_shadow = val;
> #endif
> - else if ( (val = parse_boolean("verbose", s, ss)) >= 0 )
> - opt_dom0_verbose = val;
> - else if ( IS_ENABLED(CONFIG_PV) &&
> - (val = parse_boolean("cpuid-faulting", s, ss)) >= 0 )
> - opt_dom0_cpuid_faulting = val;
> - else if ( (val = parse_boolean("msr-relaxed", s, ss)) >= 0 )
> - opt_dom0_msr_relaxed = val;
> - else
> - rc = -EINVAL;
> -
> - s = ss + 1;
> - } while ( *ss );
> + else if ( (val = parse_boolean("verbose", str_begin, str_end)) >= 0 )
> + opt_dom0_verbose = val;
> + else if ( IS_ENABLED(CONFIG_PV) &&
> + (val = parse_boolean("cpuid-faulting", str_begin, str_end)) >=
> 0 )
> + opt_dom0_cpuid_faulting = val;
> + else if ( (val = parse_boolean("msr-relaxed", str_begin, str_end)) >= 0 )
> + opt_dom0_msr_relaxed = val;
> + else
> + rc = -EINVAL;
>
> return rc;
> }
While largely a style aspect (and hence I'm not going to insist), I don't
see the value of the "rc" local variable with the changed logic. With at
least the other aspects addressed
Reviewed-by: Jan Beulich <jbeul...@suse.com>
Jan
