[libvirt] [PATCH v2 3/4] qemu: use newer -device video device in qemu commandline

[Guannan Ren]

On 12/12/2012 02:25 AM, Eric Blake wrote:
> On 12/11/2012 07:14 AM, Guannan Ren wrote:
>> '-device VGA' maps to '-vga std'
>> '-device cirrus-vga' maps to '-vga cirrus'
>> '-device qxl-vga' maps to '-vga qxl'
>>               (there is also '-device qxl' for secondary devices)
>> '-device vmware-svga' maps to '-vga vmware'
>>
>> For qemu(>=1.2), we can use -device to replace -vga for video
>> device. For the primary video device, the patch trys to use 0x2
>> slot for matching old qemu. If the 0x2 slot is allocated already,
>> the addr property could help for using any available slot.
>> For qemu(< 1.2), we keep using -vga for primary device.
>> ---
>>   src/qemu/qemu_command.c  | 132 ++++++++++++++++++++++++++++++++++++-----------
>>   src/qemu/qemu_command.h  |   4 +-
>>   tests/qemuxml2argvtest.c |   9 ++--
>>   3 files changed, 110 insertions(+), 35 deletions(-)
>>
>>   {
>>       size_t i, j;
>>       bool reservedIDE = false;
>>       bool reservedUSB = false;
>>       int function;
>> +    bool qemu1dot2plus = qemuCapsGetVersion(caps) >= 1002000;
> Eww.  Version string comparison is wrong, as it is feasible that someone
> could backport the improved command line to older qemu.  We should
> instead be going off of whether specific '-device XXX' is supported;
> probably by checking for QEMU_CAPS_DEVICE_QXL.
>
>> @@ -6427,22 +6477,42 @@ qemuBuildCommandLine(virConnectPtr conn,
>>               goto error;
>>       }
>>       if (def->nvideos > 0) {
>> -        if (qemuCapsGet(caps, QEMU_CAPS_VGA)) {
>> -            if (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_XEN) {
>> +        int primaryVideoType = def->videos[0]->type;
>> +        if (qemuCapsGetVersion(caps) >= 1002000 &&
> Again, version comparison feels wrong.  What are you really gating on,
> whether qemu is new enough to support primary video on a non-default
> address?  If so, set up the right capability bit for that.

           Although '-device XXX'  is there but it probably doesn't work 
for qemu(<1.2)
           the following is the comments from  Gerd Hoffmann in v1

         "That will not work.  You must check the qemu version.  1.2+ is safe,
         IIRC 1.1 will work too.

         Technical background: '-device VGA' (+variants) exists for a looooooong
         time, pretty much day one of the -device switch.  Actually using it did
         not work though due to a nasty initialization order issue in qemu.  The
         qemu memory api created by avi fixed it."




>
>> +             ((primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VGA &&
>> +                 qemuCapsGet(caps, QEMU_CAPS_DEVICE_VGA)) ||
>> +             (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_CIRRUS &&
>> +                 qemuCapsGet(caps, QEMU_CAPS_DEVICE_CIRRUS_VGA)) ||
>> +             (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VMVGA &&
>> +                 qemuCapsGet(caps, QEMU_CAPS_DEVICE_VMWARE_SVGA)) ||
>> +             (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_QXL &&
>> +                 qemuCapsGet(caps, QEMU_CAPS_DEVICE_QXL_VGA)))
> Or is the existence of these four device capability bits sufficient?
>

            device capability bits checking here is in case that one of 
four video device
            is missing on some qemu(>=1.2) though.