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

[Martin Kletzander]

On Fri, Feb 07, 2014 at 01:10:42PM -0700, Eric Blake wrote:
> 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 at 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?
>

Adding the parsing capability doesn't change anything for qemu >= 1.2
or for qemu < 1.2, but if somebody backports spiceport into qemu <
1.2, this will make it work.

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

You're right, I should've added more comments about that.  The thing
is that it is wired into QAPI in a sense that it can be queried as an
object in running domain or hot(un)plugged, but it cannot be queried
for as a capability (hence my patch for qemu that lists chardev
backends.

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

OK, I'll split these into 2.

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

No problem with v3, at least it'll be cleaner.  Thanks for the review.

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140210/1dad079b/attachment-0001.sig>