On 02/07/2014 04:37 AM, Martin Kletzander wrote:
Adding a new backend that makes the chardev available to be backed up by a port in spice connection (different to spicevmc). This can be used (as well as other backends) for any chardev libvirt supports.
Apart from spicevmc, spiceport-backed chardev will not be formatted into the command-line if there is no spice to use (with test for that as well). For this I moved the def->graphics caounting to the start
s/caounting/counting/
of the function so its results can be used in rest of the code even in the future.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
--- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c
Hmm, I might have split this into two patches - one for the XML and docs, and one for the qemu implementation.
@@ -1012,6 +1013,8 @@ virQEMUCapsComputeCmdFlags(const char *help, virQEMUCapsSet(qemuCaps, QEMU_CAPS_CHARDEV); if (strstr(help, "-chardev spicevmc")) virQEMUCapsSet(qemuCaps, QEMU_CAPS_CHARDEV_SPICEVMC); + if (strstr(help, "-chardev spiceport")) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_CHARDEV_SPICEPORT);
Why are we parsing -help output? Does qemu < 1.2 support spiceport?
} if (strstr(help, "-balloon")) virQEMUCapsSet(qemuCaps, QEMU_CAPS_BALLOON); @@ -2567,6 +2570,11 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, if (qemuCaps->version >= 1003001) virQEMUCapsSet(qemuCaps, QEMU_CAPS_VNC_WEBSOCKET);
+ /* -chardev spiceport is supported from 1.4.0,
Especially given this comment, we should NOT parse -help output.
+ * but it's in qapi only since 1.5.0 */ + if (qemuCaps->version >= 1005000) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_CHARDEV_SPICEPORT);
So why are we hard-coding a version number instead of querying qapi? This is all the more reason to split the commit - the XML and doc and src/conf changes are fine, but the qemu side needs work or more comments explaining why we are using version numbers instead of feature probing.
@@ -8802,35 +8831,39 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArgBuffer(cmd, &opt); }
- if (!def->nserials) { - /* If we have -device, then we set -nodefault already */ - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) - virCommandAddArgList(cmd, "-serial", "none", NULL); - } else { - for (i = 0; i < def->nserials; i++) { - virDomainChrDefPtr serial = def->serials[i]; - char *devstr; + for (i = 0; i < def->nserials; i++) { + virDomainChrDefPtr serial = def->serials[i]; + char *devstr;
This hunk was a bit hard to follow; in cases like this, I will sometimes split into two patches - one that changes the logic but leaves indentation off (or adds {}) so that the just the logic change is shown, then the second that fixes indentation. The combined diff is the same, but it is much easier to review the two halves.
+++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-spiceport-nospice.xml @@ -0,0 +1,40 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <title>A description of the test machine.</title> + <description> + A test of qemu's minimal configuration. + This test also tests the description and title elements.
Copied and pasted from another test :) Not a problem, but also not minimal for testing this particular XML. Thanks for the tests. Probably worth a v3, or at least a better explanation of how the qemu capability setting is done, before I feel good with ack. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org