[libvirt] [PATCH 08/11] qemu_command: cleanup qemuBuildVideoCommandLine

John Ferlan jferlan at redhat.com
Sat Oct 8 14:01:33 UTC 2016



On 09/30/2016 12:02 PM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> ---
>  src/qemu/qemu_command.c | 69 ++++++++++++++++++++++++++++---------------------
>  1 file changed, 40 insertions(+), 29 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index aef8c79..15bb381 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4451,43 +4451,54 @@ qemuBuildVideoCommandLine(virCommandPtr cmd,
>                            virQEMUCapsPtr qemuCaps)
>  {
>      size_t i;
> -    int primaryVideoType;
> +    char *str = NULL;
>  
> -    if (!def->videos)
> -        return 0;
> +    for (i = 0; i < def->nvideos; i++) {
> +        virDomainVideoDefPtr video = def->videos[i];
>  
> -    primaryVideoType = def->videos[0]->type;
> +        switch (video->type) {
> +        case VIR_DOMAIN_VIDEO_TYPE_VGA:
> +        case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
> +        case VIR_DOMAIN_VIDEO_TYPE_VMVGA:
> +        case VIR_DOMAIN_VIDEO_TYPE_VIRTIO:
> +        case VIR_DOMAIN_VIDEO_TYPE_QXL:
> +            if (video->primary) {

So thinking back to my patch 5 comment - this essentially assumes
def->videos[0] and video->primary are equivalent, right?

> +                if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY)) {
>  
> -    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY)) {
> -        for (i = 0; i < def->nvideos; i++) {
> -            char *str;
> -            virCommandAddArg(cmd, "-device");
> -            if (!(str = qemuBuildDeviceVideoStr(def, def->videos[i],
> -                                                qemuCaps)))
> -                return -1;
> +                    virCommandAddArg(cmd, "-device");
>  
> -            virCommandAddArg(cmd, str);
> -            VIR_FREE(str);
> -        }
> -    } else {
> -        if (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_XEN) {
> -            /* nothing - vga has no effect on Xen pvfb */
> -        } else {
> -            if (qemuBuildVgaVideoCommand(cmd, def, qemuCaps) < 0)
> -                return -1;
> -        }
> +                    if (!(str = qemuBuildDeviceVideoStr(def, def->videos[i],

This would be "video", not the def->videos[i]

> +                                                        qemuCaps)))
> +                        return -1;
> +
> +                    virCommandAddArg(cmd, str);
> +                    VIR_FREE(str);
> +                } else {
> +                    if (qemuBuildVgaVideoCommand(cmd, def, qemuCaps) < 0)

And from the previous patch passing video here still works

> +                        return -1;
> +                }
> +            } else {
> +                virCommandAddArg(cmd, "-device");
>  
> -        for (i = 1; i < def->nvideos; i++) {
> -            char *str;
> +                if (!(str = qemuBuildDeviceVideoStr(def, def->videos[i],

Likewise "video" not the def->videos[i]

> +                                                    qemuCaps)))
> +                    return -1;
>  
> -            virCommandAddArg(cmd, "-device");
> +                virCommandAddArg(cmd, str);
> +                VIR_FREE(str);
> +            }
> +            break;
>  
> -            if (!(str = qemuBuildDeviceVideoStr(def, def->videos[i],
> -                                                qemuCaps)))
> -                return -1;
> +        case VIR_DOMAIN_VIDEO_TYPE_VBOX:
> +        case VIR_DOMAIN_VIDEO_TYPE_XEN:

Doesn't putting this here break the previous else check of:

-        if (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_XEN) {
-            /* nothing - vga has no effect on Xen pvfb */

IOW: Shouldn't the XEN type code check code "skip" def->videos[0] (or
video->primary)?

ACK with the proper usage of video not def->videos[i] and making sure
the _XEN case is handled properly


John

> +        case VIR_DOMAIN_VIDEO_TYPE_PARALLELS:
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("video type '%s' is not supported with QEMU"),
> +                           virDomainVideoTypeToString(video->type));
> +            return -1;
>  
> -            virCommandAddArg(cmd, str);
> -            VIR_FREE(str);
> +        case VIR_DOMAIN_VIDEO_TYPE_LAST:
> +            break;
>          }
>      }
>  
> 




More information about the libvir-list mailing list