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

[Francesco Romani]

Hi, thanks for the review

> 
> I'd use a wording similar to what is used for copy&paste:
> "File transfer functionality (via Spice agent) is set using the
> <code>filetransfer</code> element. It is enabled by default, and can be
> disabled by setting the
> <code>copypaste</code> property to <code>no</code>", or at least I'd reword
> the first sentence which is a bit confusing imo, and is mostly implementation
> details.
> At this point, I think it will be "since 1.2.2"

fine for me, I will fix both.

> > virDomainGraphicsSpiceAgentFileTransferTypeFromString(enable))
> > <= 0) {
> > +                        virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                                       _("unknown enable value '%s'"),
> > enable);
> 
> the code to disable copy&paste uses VIR_ERR_INTERNAL_ERROR when it's not
> able to parse the enum value, but the compression code uses
> VIR_ERR_CONFIG_UNSUPPORTED, and the mouse code uses VIR_ERR_XML_ERROR :(
> The rest of domain_conf.c is not very consistent in what error is reported
> on unknown enum values :( I'd personnally go with either
> VIR_ERR_CONFIG_UNSUPPORTED or VIR_ERR_XML_ERROR as they are more specific
> than VIR_ERR_INTERNAL_ERROR. However, given that the rest of the code is
> inconsistent, this can stay this way for now, and be fixed up at a later
> time.

I already have to resubmit, and I personally like errors to be specific,
so I will change to VIR_ERR_CONFIG_UNSUPPORTED, because it seems the most
relevant.

> 
> > virDomainGraphicsSpiceAgentFileTransferTypeToString(def->data.spice.copypaste));
> 
> s/copypaste/filetransfer/

Ops :) will fix.

> > +           virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +                          _("This QEMU can't disable the spice file
> > transfer"));
> 
> I'd say "spice file transfers" or "file transfers through spice" rather
> than "the spice file transfer"

Ok for "file transfers through spice".

New version of the patch coming soon.

Thanks and best regards,

-- 
Francesco Romani