Re: [libvirt] [PATCH v2] qemu: introduce spiceport serial backend
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(a)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
Attachments:
- signature.asc (application/pgp-signature — 604 bytes)