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

Francesco Romani fromani at redhat.com
Mon Jan 6 09:55:53 UTC 2014


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

More information about the libvir-list mailing list