Re: [Qemu-devel] [PATCH 1/2] vl: Table-based select_vgahw()

[Eduardo Habkost]

On Wed, Nov 11, 2015 at 03:05:07PM -0700, Eric Blake wrote:
> On 11/11/2015 02:27 PM, Eduardo Habkost wrote:
> > Instead of implementing separate check functions for each vga
> > interface type, add a table enumerating the possible VGA
> > interfaces.
> > 
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> >  include/sysemu/sysemu.h |   1 +
> >  vl.c                    | 114 
> > ++++++++++++++++++++++++++----------------------
> >  2 files changed, 63 insertions(+), 52 deletions(-)
> > 
> > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> > index f992494..6406906 100644
> > --- a/include/sysemu/sysemu.h
> > +++ b/include/sysemu/sysemu.h
> > @@ -147,6 +147,7 @@ extern int autostart;
> >  typedef enum {
> >      VGA_NONE, VGA_STD, VGA_CIRRUS, VGA_VMWARE, VGA_XENFB, VGA_QXL,
> >      VGA_TCX, VGA_CG3, VGA_DEVICE, VGA_VIRTIO,
> > +    VGA_TYPE_MAX,
> >  } VGAInterfaceType;
> 
> Would it be worth exposing this enum in qapi someday? But doesn't affect
> the correctness of this patch.

I don't know. Maybe not, if we have any plans to configure this
using QOM classes somehow (like the CPU model stuff), not using
enums.

> 
> >  static void select_vgahw (const char *p)
> 
> Worth dropping the space before '(' while in the neighborhood?
> 
> >  {
> >      const char *opts;
> > +    int t;
> >  
> > -    assert(vga_interface_type == VGA_NONE);
> 
> Are you intentionally dropping the assert?  It protects us from
> select_vgahw() being called more than once.

That was not intentional. I will fix it in a new version. Thanks!

> 
> > -    } else if (strstart(p, "cg3", &opts)) {
> > -        if (cg3_vga_available()) {
> > -            vga_interface_type = VGA_CG3;
> > -        } else {
> > -            error_report("CG3 framebuffer not available");
> > -            exit(1);
> > +    for (t = 0; t < VGA_TYPE_MAX; t++) {
> > +        VGAInterfaceInfo *ti = &vga_interfaces[t];
> > +        if (ti->opt_name && strstart(p, ti->opt_name, &opts)) {
> > +            if (ti->available && !ti->available()) {
> > +                error_report("%s not available", ti->name);
> > +                exit(1);
> > +            }
> > +            vga_interface_type = t;
> > +            break;
> 
> Nice compression in code size.
> 
> Reviewed-by: Eric Blake <address@hidden>

Thanks!

-- 
Eduardo