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

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
 
-- 
2.41.0

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