| Message ID | 20170816125219.5255-21-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 16.08.17 at 14:51, <jgross@suse.com> wrote: > --- a/xen/arch/x86/shutdown.c > +++ b/xen/arch/x86/shutdown.c > @@ -51,8 +51,11 @@ static int reboot_mode; > * efi Use the EFI reboot (if running under EFI) > */ > static enum reboot_type reboot_type = BOOT_INVALID; > -static void __init set_reboot_type(char *str) > + > +static int __init set_reboot_type(const char *str) > { > + int rc = 0; > + > for ( ; ; ) > { > switch ( *str ) > @@ -74,6 +77,8 @@ static void __init set_reboot_type(char *str) > case 't': > reboot_type = *str; > break; > + default: > + rc = -EINVAL; > } Please don't omit the break statement, even if it is not strictly needed here. > @@ -82,6 +87,8 @@ static void __init set_reboot_type(char *str) > > if ( reboot_type == BOOT_EFI && !efi_enabled(EFI_RS) ) > reboot_type = BOOT_INVALID; Should this perhaps also lead to -EINVAL being returned? Jan
On 22/08/17 11:53, Jan Beulich wrote: >>>> On 16.08.17 at 14:51, <jgross@suse.com> wrote: >> --- a/xen/arch/x86/shutdown.c >> +++ b/xen/arch/x86/shutdown.c >> @@ -51,8 +51,11 @@ static int reboot_mode; >> * efi Use the EFI reboot (if running under EFI) >> */ >> static enum reboot_type reboot_type = BOOT_INVALID; >> -static void __init set_reboot_type(char *str) >> + >> +static int __init set_reboot_type(const char *str) >> { >> + int rc = 0; >> + >> for ( ; ; ) >> { >> switch ( *str ) >> @@ -74,6 +77,8 @@ static void __init set_reboot_type(char *str) >> case 't': >> reboot_type = *str; >> break; >> + default: >> + rc = -EINVAL; >> } > > Please don't omit the break statement, even if it is not strictly needed > here. Okay. > >> @@ -82,6 +87,8 @@ static void __init set_reboot_type(char *str) >> >> if ( reboot_type == BOOT_EFI && !efi_enabled(EFI_RS) ) >> reboot_type = BOOT_INVALID; > > Should this perhaps also lead to -EINVAL being returned? Hmm, I'm not sure. The parameter as such was valid. So maybe a message right here would be the better solution? Juergen
>>> On 23.08.17 at 10:40, <jgross@suse.com> wrote: > On 22/08/17 11:53, Jan Beulich wrote: >>>>> On 16.08.17 at 14:51, <jgross@suse.com> wrote: >>> @@ -82,6 +87,8 @@ static void __init set_reboot_type(char *str) >>> >>> if ( reboot_type == BOOT_EFI && !efi_enabled(EFI_RS) ) >>> reboot_type = BOOT_INVALID; >> >> Should this perhaps also lead to -EINVAL being returned? > > Hmm, I'm not sure. The parameter as such was valid. > > So maybe a message right here would be the better solution? I'd be fine with that too, it's just that with the overall change your series does this shouldn't go silent anymore. Jan
diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c index f63b8a668f..862384a8eb 100644 --- a/xen/arch/x86/shutdown.c +++ b/xen/arch/x86/shutdown.c @@ -51,8 +51,11 @@ static int reboot_mode; * efi Use the EFI reboot (if running under EFI) */ static enum reboot_type reboot_type = BOOT_INVALID; -static void __init set_reboot_type(char *str) + +static int __init set_reboot_type(const char *str) { + int rc = 0; + for ( ; ; ) { switch ( *str ) @@ -74,6 +77,8 @@ static void __init set_reboot_type(char *str) case 't': reboot_type = *str; break; + default: + rc = -EINVAL; } if ( (str = strchr(str, ',')) == NULL ) break; @@ -82,6 +87,8 @@ static void __init set_reboot_type(char *str) if ( reboot_type == BOOT_EFI && !efi_enabled(EFI_RS) ) reboot_type = BOOT_INVALID; + + return rc; } custom_param("reboot", set_reboot_type);
Modify the custom parameter parsing routines in: xen/arch/x86/shutdown.c to indicate whether the parameter value was parsed successfully. Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Juergen Gross <jgross@suse.com> --- V3: - dont stop loop at first invalid character (Jan Beulich) --- xen/arch/x86/shutdown.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)