Re: [PATCH v3 1/3] multiboot2: parse vga= option when setting GOP mode

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jun 07, 2023 at 12:10:28PM +0200, Jan Beulich wrote:
> On 01.06.2023 15:05, Roger Pau Monne wrote:
> > @@ -807,7 +830,41 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, 
> > EFI_SYSTEM_TABLE *SystemTable
> >  
> >      if ( gop )
> >      {
> > -        gop_mode = efi_find_gop_mode(gop, 0, 0, 0);
> > +        const char *last = cmdline;
> > +        unsigned int width = 0, height = 0, depth = 0;
> > +        bool keep_current = false;
> > +
> > +        while ( (last = get_option(last, "vga=")) != NULL )
> > +        {
> > +            if ( !strncmp(last, "gfx-", 4) )
> > +            {
> > +                width = simple_strtoul(last + 4, &last, 10);
> > +                if ( *last == 'x' )
> > +                    height = simple_strtoul(last + 1, &last, 10);
> > +                if ( *last == 'x' )
> > +                    depth = simple_strtoul(last + 1, &last, 10);
> > +                if ( *last != ' ' && *last != '\t' && *last != '\0' &&
> > +                     *last != ',' )
> 
> You check for an appropriate terminator here.
> 
> > +                    width = height = depth = 0;
> > +                keep_current = false;
> > +            }
> > +            else if ( !strncmp(last, "current", 7) )
> 
> Don't you also need to do so here, and maybe even ...
> 
> > +                keep_current = true;
> > +            else if ( !strncmp(last, "keep", 4) )
> > +            {
> > +                /* Ignore. */
> 
> ... here?

Hm, quite possibly for correctness.  I felt it was relevant in gfx- as
to avoid things like: gfx-1024x786x32x64 being handled, but the same
could apply to passing on option like current-bar.

Will try to generalize the terminator parsing so it applies to all
options.

Thanks, Roger.