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&gt; > --- > > --- 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&apos;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.