| Message ID | 20230710141238.375-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cmdline: only set ask mode if vga= is present | expand |
On 10.07.2023 16:12, Roger Pau Monne wrote: > Commit 9473d9a24182 set the ASK mode without checking if there was a > `vga` option provided in the command line. This breaks existing > behavior, so exit early without changes if `vga` is not present in the > command line. > > Fixes: 9473d9a24182 ('cmdline: parse multiple instances of the vga option') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> Should have spotted this during review; effectively you're (almost) undoing part of the earlier change, just that ... > --- a/xen/arch/x86/boot/cmdline.c > +++ b/xen/arch/x86/boot/cmdline.c > @@ -279,9 +279,13 @@ static void vga_parse(const char *cmdline, early_boot_opts_t *ebo) > { > const char *c = cmdline; > > + c = find_opt(c, "vga=", true); ... you use c instead of cmdline here (and I'm heavily tempted to actually make this the initializer of c). Jan > + if ( !c ) > + return; > + > ebo->boot_vid_mode = ASK_VGA; > > - while ( (c = find_opt(c, "vga=", true)) != NULL ) > + do > { > unsigned int tmp, vesa_depth, vesa_height, vesa_width; > > @@ -332,6 +336,7 @@ static void vga_parse(const char *cmdline, early_boot_opts_t *ebo) > else if ( !strmaxcmp(c, "ask", delim_chars_comma) ) > ebo->boot_vid_mode = ASK_VGA; > } > + while ( (c = find_opt(c, "vga=", true)) != NULL ); > } > #endif >
On Mon, Jul 10, 2023 at 06:27:06PM +0200, Jan Beulich wrote: > On 10.07.2023 16:12, Roger Pau Monne wrote: > > Commit 9473d9a24182 set the ASK mode without checking if there was a > > `vga` option provided in the command line. This breaks existing > > behavior, so exit early without changes if `vga` is not present in the > > command line. > > > > Fixes: 9473d9a24182 ('cmdline: parse multiple instances of the vga option') > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > Should have spotted this during review; effectively you're (almost) undoing > part of the earlier change, just that ... > > > --- a/xen/arch/x86/boot/cmdline.c > > +++ b/xen/arch/x86/boot/cmdline.c > > @@ -279,9 +279,13 @@ static void vga_parse(const char *cmdline, early_boot_opts_t *ebo) > > { > > const char *c = cmdline; > > > > + c = find_opt(c, "vga=", true); > > ... you use c instead of cmdline here (and I'm heavily tempted to actually > make this the initializer of c). I see, yes, please do. Thanks, Roger.
On 11.07.2023 11:14, Roger Pau Monné wrote: > On Mon, Jul 10, 2023 at 06:27:06PM +0200, Jan Beulich wrote: >> On 10.07.2023 16:12, Roger Pau Monne wrote: >>> Commit 9473d9a24182 set the ASK mode without checking if there was a >>> `vga` option provided in the command line. This breaks existing >>> behavior, so exit early without changes if `vga` is not present in the >>> command line. >>> >>> Fixes: 9473d9a24182 ('cmdline: parse multiple instances of the vga option') >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> >> Should have spotted this during review; effectively you're (almost) undoing >> part of the earlier change, just that ... >> >>> --- a/xen/arch/x86/boot/cmdline.c >>> +++ b/xen/arch/x86/boot/cmdline.c >>> @@ -279,9 +279,13 @@ static void vga_parse(const char *cmdline, early_boot_opts_t *ebo) >>> { >>> const char *c = cmdline; >>> >>> + c = find_opt(c, "vga=", true); >> >> ... you use c instead of cmdline here (and I'm heavily tempted to actually >> make this the initializer of c). > > I see, yes, please do. Well, no, I didn't (without your consent), and I wanted to get the patch in yesterday before leaving. Jan
diff --git a/xen/arch/x86/boot/cmdline.c b/xen/arch/x86/boot/cmdline.c index 10dcc6142c85..74997703b31e 100644 --- a/xen/arch/x86/boot/cmdline.c +++ b/xen/arch/x86/boot/cmdline.c @@ -279,9 +279,13 @@ static void vga_parse(const char *cmdline, early_boot_opts_t *ebo) { const char *c = cmdline; + c = find_opt(c, "vga=", true); + if ( !c ) + return; + ebo->boot_vid_mode = ASK_VGA; - while ( (c = find_opt(c, "vga=", true)) != NULL ) + do { unsigned int tmp, vesa_depth, vesa_height, vesa_width; @@ -332,6 +336,7 @@ static void vga_parse(const char *cmdline, early_boot_opts_t *ebo) else if ( !strmaxcmp(c, "ask", delim_chars_comma) ) ebo->boot_vid_mode = ASK_VGA; } + while ( (c = find_opt(c, "vga=", true)) != NULL ); } #endif
Commit 9473d9a24182 set the ASK mode without checking if there was a `vga` option provided in the command line. This breaks existing behavior, so exit early without changes if `vga` is not present in the command line. Fixes: 9473d9a24182 ('cmdline: parse multiple instances of the vga option') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Does seem to fix the broken gitlab tests: https://gitlab.com/xen-project/people/royger/xen/-/pipelines/926397265 --- xen/arch/x86/boot/cmdline.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)