Re: [Qemu-devel] [PATCH for-2.7 v2 2/3] vl: Table-based select_vgahw()

[Marcel Apfelbaum <marcel@redhat.com>]

On 04/14/2016 03:51 AM, Eduardo Habkost wrote:
> Instead of implementing separate check functions for each vga
> interface type, add a table enumerating the possible VGA
> interfaces.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v1 -> v2:
> * Keep assert(vga_interface_type == VGA_NONE) line
>    Suggested-by: Eric Blake <eblake@redhat.com>
> * Fix spacing on select_vgahw() definition
>    Suggested-by: Eric Blake <eblake@redhat.com>
> ---
>   include/sysemu/sysemu.h |   1 +
>   vl.c                    | 115 ++++++++++++++++++++++++++----------------------
>   2 files changed, 64 insertions(+), 52 deletions(-)
>
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 38fb3ca..2033f8c 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -146,6 +146,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;
>
>   extern int vga_interface_type;
> diff --git a/vl.c b/vl.c
> index 3c565bb..b99f22f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2015,63 +2015,74 @@ static bool virtio_vga_available(void)
>       return object_class_by_name("virtio-vga");
>   }
>
> -static void select_vgahw (const char *p)
> +typedef struct VGAInterfaceInfo {
> +    const char *opt_name;    /* option name */
> +    const char *name;        /* human-readable name */
> +    bool (*available)(void);
> +} VGAInterfaceInfo;
> +
> +static VGAInterfaceInfo vga_interfaces[VGA_TYPE_MAX] = {
> +    [VGA_NONE] = {
> +        .opt_name = "none",
> +    },
> +    [VGA_STD] = {
> +        .opt_name = "std",
> +        .name = "standard VGA",
> +        .available = vga_available,
> +    },
> +    [VGA_CIRRUS] = {
> +        .opt_name = "cirrus",
> +        .name = "Cirrus VGA",
> +        .available = cirrus_vga_available,
> +    },
> +    [VGA_VMWARE] = {
> +        .opt_name = "vmware",
> +        .name = "VMWare SVGA",
> +        .available = vmware_vga_available,
> +    },
> +    [VGA_VIRTIO] = {
> +        .opt_name = "virtio",
> +        .name = "Virtio VGA",
> +        .available = virtio_vga_available,
> +    },
> +    [VGA_QXL] = {
> +        .opt_name = "qxl",
> +        .name = "QXL VGA",
> +        .available = qxl_vga_available,
> +    },
> +    [VGA_TCX] = {
> +        .opt_name = "tcx",
> +        .name = "TCX framebuffer",
> +        .available = tcx_vga_available,
> +    },
> +    [VGA_CG3] = {
> +        .opt_name = "cg3",
> +        .name = "CG3 framebuffer",
> +        .available = cg3_vga_available,
> +    },
> +    [VGA_XENFB] = {
> +        .opt_name = "xenfb",
> +    },
> +};
> +
> +static void select_vgahw(const char *p)
>   {
>       const char *opts;
> +    int t;
>
>       assert(vga_interface_type == VGA_NONE);
> -    if (strstart(p, "std", &opts)) {
> -        if (vga_available()) {
> -            vga_interface_type = VGA_STD;
> -        } else {
> -            error_report("standard VGA not available");
> -            exit(1);
> -        }
> -    } else if (strstart(p, "cirrus", &opts)) {
> -        if (cirrus_vga_available()) {
> -            vga_interface_type = VGA_CIRRUS;
> -        } else {
> -            error_report("Cirrus VGA not available");
> -            exit(1);
> -        }
> -    } else if (strstart(p, "vmware", &opts)) {
> -        if (vmware_vga_available()) {
> -            vga_interface_type = VGA_VMWARE;
> -        } else {
> -            error_report("VMWare SVGA not available");
> -            exit(1);
> -        }
> -    } else if (strstart(p, "virtio", &opts)) {
> -        if (virtio_vga_available()) {
> -            vga_interface_type = VGA_VIRTIO;
> -        } else {
> -            error_report("Virtio VGA not available");
> -            exit(1);
> -        }
> -    } else if (strstart(p, "xenfb", &opts)) {
> -        vga_interface_type = VGA_XENFB;
> -    } else if (strstart(p, "qxl", &opts)) {
> -        if (qxl_vga_available()) {
> -            vga_interface_type = VGA_QXL;
> -        } else {
> -            error_report("QXL VGA not available");
> -            exit(1);
> -        }
> -    } else if (strstart(p, "tcx", &opts)) {
> -        if (tcx_vga_available()) {
> -            vga_interface_type = VGA_TCX;
> -        } else {
> -            error_report("TCX framebuffer not available");
> -            exit(1);
> -        }
> -    } 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;
>           }
> -    } else if (!strstart(p, "none", &opts)) {
> +    }
> +    if (t == VGA_TYPE_MAX) {
>       invalid_vga:
>           error_report("unknown vga type: %s", p);
>           exit(1);
>

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel