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

On 12/11/2012 08:32 PM, Guannan Ren wrote: >>> + 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. Still, I'd rather see us create a capability bit, even if that bit is initially set according to a version check in qemu_capabilities.c, and use that bit rather than polluting the rest of the qemu code with additional version checks. That way, if someone manages to backport fixed device handling even to RHEL qemu 0.12.x, as well as advertise that backport, we can set the bit when the backport is detected. It may even be as simple as a RHEL-specific patch that merely blindly sets the capability bit at the one place where upstream sets the bit according to a version check. But no matter how we approach it, having a dedicated capability bit rather than multiple version checks will make backporting much easier.