Hello, ----- Original Message ----- > From: "Eric Blake" <eblake@xxxxxxxxxx> > To: "Francesco Romani" <fromani@xxxxxxxxxx>, libvir-list@xxxxxxxxxx > Sent: Saturday, January 4, 2014 3:33:09 PM > Subject: Re: [PATCH] spice: expose the disable file transfer option > > On 01/02/2014 02:59 AM, Francesco Romani wrote: > > spice-server offers an API to disable file transfer messages > > on the agent channel between the client and the guest. > > This is supported in qemu through the disable-agent-file-xfer option. > > > > This patch exposes this option to libvirt. > > --- > > src/conf/domain_conf.c | 26 ++++++++++++++++++++++++++ > > src/conf/domain_conf.h | 10 ++++++++++ > > src/libvirt_private.syms | 2 ++ > > src/qemu/qemu_command.c | 2 ++ > > 4 files changed, 40 insertions(+) > > Thanks for starting this patch. Since this adds new XML, you also need > to patch docs/formatdomain.html.in to document it, > docs/schemas/domaincommon.rng to allow 'virt-xml-validate' to recognize > it, and tests/qemuxml2* to add a new testsuite that proves we can parse > the new XML as valid, round-trip it through our internal representation, > and generate the correct command line option. Also, it helps if the > commit message describes the actual XML you added to expose the option. > > Are you able to help add those pieces to get this patch incorporated? Yes, I'll go ahead. I'm not sure the XML I added is the best way to express the option (I have no strong preference on this topic, however), but I think we can discuss this after the bits you outlined are in place. > > > @@ -8519,6 +8525,26 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, > > VIR_FREE(copypaste); > > > > You added a parser for the new function, but did not modify the output > function, which means your action is one-way; it won't survive a round > trip to 'virsh dumpxml' and would be lost the next time we use the > domain. Again, that's why we insist on testsuite additions for new XML > features. Ugh, this was unintentional, thanks for pointing this out, I'll update the patch. [...] > It is probably also wise to split this into two patches. Not all > versions of qemu support the new option, so we probably want to add a > feature bit into qemu_capabilities.[hc] that probes when we can safely > use the feature, so that we can issue a graceful error for qemu versions > that lack it. In which case you want two patches - one for the XML > addition, and one for the qemu capability addition. Fine for me, I'll update the patch(set) accordingly and repost once ready. Thanks for the review and happy new year 2014, -- Francesco Romani -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list