Re: [PATCH v2] qemu: introduce spiceport serial backend

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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&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.

-- 
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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]