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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list