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@xxxxxxxxxx>
> ---
>
> --- 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list