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