[libvirt] [PATCH] spice: expose the disable file transfer option

[Francesco Romani]

Hello,

----- Original Message -----
> From: "Eric Blake" <eblake at redhat.com>
> To: "Francesco Romani" <fromani at redhat.com>, libvir-list at redhat.com
> Sent: Saturday, January 4, 2014 3:33:09 PM
> Subject: Re: [libvirt] [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