[libvirt] [PATCH v2] qemu: introduce spiceport serial backend
Martin Kletzander
mkletzan at redhat.com
Mon Feb 10 07:38:13 UTC 2014
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>
More information about the libvir-list
mailing list