[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Tue, Jan 07, 2014 at 08:04:33PM +0100, 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.
> Adds a new element 'filetransfer', with one property,
> 'filetransfer', which accepts a boolean setting.
> Default is enabled.
> 
> Depends on the capability exported in the first patch of the series.
> ---
>  docs/formatdomain.html.in                          |  8 +++++
>  docs/schemas/domaincommon.rng                      | 11 ++++++
>  src/conf/domain_conf.c                             | 31 ++++++++++++++++-
>  src/conf/domain_conf.h                             | 10 ++++++
>  src/libvirt_private.syms                           |  2 ++
>  src/qemu/qemu_command.c                            |  9 +++++
>  ...emuxml2argv-graphics-spice-agent-file-xfer.args |  9 +++++
>  ...qemuxml2argv-graphics-spice-agent-file-xfer.xml | 40 ++++++++++++++++++++++
>  tests/qemuxml2argvtest.c                           |  6 ++++
>  9 files changed, 125 insertions(+), 1 deletion(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-agent-file-xfer.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-agent-file-xfer.xml
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 68860ef..f3186e8 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -4042,6 +4042,7 @@ qemu-kvm -net nic,model=? /dev/null
>      <streaming mode='filter'/>
>      <clipboard copypaste='no'/>
>      <mouse mode='client'/>
> +    <filetransfer enable='no'/>
>    &lt;/graphics&gt;</pre>
>              <p>
>                Spice supports variable compression settings for audio,
> @@ -4081,6 +4082,13 @@ qemu-kvm -net nic,model=? /dev/null
>                <span class="since">since 0.9.11</span>. If no mode is
>                specified, the qemu default will be used (client mode).
>              </p>
> +            <p>
> +              File transfer on the agent channel between the client and
> +              the guest is supported through spice-server and enabled
> +              by default. It can be disabled by setting the <code>enable</code>
> +              property to <code>no</code> in the <code>filetransfer</code>
> +              element, <span class="since">since 1.2.1</span>.

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"

> +            </p>
>            </dd>
>            <dt><code>"rdp"</code></dt>
>            <dd>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 86a60c9..cd2c499 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -2468,6 +2468,17 @@
>                  <empty/>
>                </element>
>              </optional>
> +            <optional>
> +              <element name="filetransfer">
> +                <attribute name="enable">
> +                  <choice>
> +                    <value>yes</value>
> +                    <value>no</value>
> +                  </choice>
> +                </attribute>
> +                <empty/>
> +              </element>
> +            </optional>
>            </interleave>
>          </group>
>          <group>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index e65f3e3..18b7759 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -604,6 +604,12 @@ VIR_ENUM_IMPL(virDomainGraphicsSpiceClipboardCopypaste,
>                "yes",
>                "no");
>  
> +VIR_ENUM_IMPL(virDomainGraphicsSpiceAgentFileTransfer,
> +              VIR_DOMAIN_GRAPHICS_SPICE_AGENT_FILE_TRANSFER_LAST,
> +              "default",
> +              "yes",
> +              "no");
> +
>  VIR_ENUM_IMPL(virDomainHostdevMode, VIR_DOMAIN_HOSTDEV_MODE_LAST,
>                "subsystem",
>                "capabilities")
> @@ -8519,6 +8525,26 @@ virDomainGraphicsDefParseXML(xmlNodePtr node,
>                      VIR_FREE(copypaste);
>  
>                      def->data.spice.copypaste = copypasteVal;
> +                } else if (xmlStrEqual(cur->name, BAD_CAST "filetransfer")) {
> +                    char *enable = virXMLPropString(cur, "enable");
> +                    int enableVal;
> +
> +                    if (!enable) {
> +                        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                                       _("spice filetransfer missing enable"));
> +                        goto error;
> +                    }
> +
> +                    if ((enableVal =
> +                         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.

> +                        VIR_FREE(enable);
> +                        goto error;
> +                    }
> +                    VIR_FREE(enable);
> +
> +                    def->data.spice.filetransfer = enableVal;
>                  } else if (xmlStrEqual(cur->name, BAD_CAST "mouse")) {
>                      char *mode = virXMLPropString(cur, "mode");
>                      int modeVal;
> @@ -16423,7 +16449,7 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
>          if (!children && (def->data.spice.image || def->data.spice.jpeg ||
>                            def->data.spice.zlib || def->data.spice.playback ||
>                            def->data.spice.streaming || def->data.spice.copypaste ||
> -                          def->data.spice.mousemode)) {
> +                          def->data.spice.mousemode || def->data.spice.filetransfer)) {
>              virBufferAddLit(buf, ">\n");
>              children = true;
>          }
> @@ -16448,6 +16474,9 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
>          if (def->data.spice.copypaste)
>              virBufferAsprintf(buf, "      <clipboard copypaste='%s'/>\n",
>                                virDomainGraphicsSpiceClipboardCopypasteTypeToString(def->data.spice.copypaste));
> +        if (def->data.spice.filetransfer)
> +            virBufferAsprintf(buf, "      <filetransfer enable='%s'/>\n",
> +                              virDomainGraphicsSpiceAgentFileTransferTypeToString(def->data.spice.copypaste));

s/copypaste/filetransfer/


>      }
>  
>      if (children) {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 647d115..ce877fc 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1461,6 +1461,14 @@ enum virDomainGraphicsSpiceClipboardCopypaste {
>      VIR_DOMAIN_GRAPHICS_SPICE_CLIPBOARD_COPYPASTE_LAST
>  };
>  
> +enum virDomainGraphicsSpiceAgentFileTransfer {
> +    VIR_DOMAIN_GRAPHICS_SPICE_AGENT_FILE_TRANSFER_DEFAULT = 0,
> +    VIR_DOMAIN_GRAPHICS_SPICE_AGENT_FILE_TRANSFER_YES,
> +    VIR_DOMAIN_GRAPHICS_SPICE_AGENT_FILE_TRANSFER_NO,
> +
> +    VIR_DOMAIN_GRAPHICS_SPICE_AGENT_FILE_TRANSFER_LAST
> +};
> +
>  enum virDomainGraphicsListenType {
>      VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE = 0,
>      VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS,
> @@ -1531,6 +1539,7 @@ struct _virDomainGraphicsDef {
>              int playback;
>              int streaming;
>              int copypaste;
> +            int filetransfer;
>          } spice;
>      } data;
>      /* nListens, listens, and *port are only useful if type is vnc,
> @@ -2693,6 +2702,7 @@ VIR_ENUM_DECL(virDomainInputBus)
>  VIR_ENUM_DECL(virDomainGraphics)
>  VIR_ENUM_DECL(virDomainGraphicsListen)
>  VIR_ENUM_DECL(virDomainGraphicsAuthConnected)
> +VIR_ENUM_DECL(virDomainGraphicsSpiceAgentFileTransfer)
>  VIR_ENUM_DECL(virDomainGraphicsSpiceChannelName)
>  VIR_ENUM_DECL(virDomainGraphicsSpiceChannelMode)
>  VIR_ENUM_DECL(virDomainGraphicsSpiceImageCompression)
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 3b3de15..2a9b0b1 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -235,6 +235,8 @@ virDomainGraphicsListenGetType;
>  virDomainGraphicsListenSetAddress;
>  virDomainGraphicsListenSetNetwork;
>  virDomainGraphicsListenSetType;
> +virDomainGraphicsSpiceAgentFileTransferTypeFromString;
> +virDomainGraphicsSpiceAgentFileTransferTypeToString;
>  virDomainGraphicsSpiceChannelModeTypeFromString;
>  virDomainGraphicsSpiceChannelModeTypeToString;
>  virDomainGraphicsSpiceChannelNameTypeFromString;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 35b7c67..0398675 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7390,6 +7390,15 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
>                            virDomainGraphicsSpiceStreamingModeTypeToString(graphics->data.spice.streaming));
>      if (graphics->data.spice.copypaste == VIR_DOMAIN_GRAPHICS_SPICE_CLIPBOARD_COPYPASTE_NO)
>          virBufferAddLit(&opt, ",disable-copy-paste");
> +    if (graphics->data.spice.filetransfer == VIR_DOMAIN_GRAPHICS_SPICE_AGENT_FILE_TRANSFER_NO) {
> +        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_FILE_XFER_DISABLE)) {
> +           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"


Christophe

Attachment: pgpcO79LWpqd3.pgp
Description: PGP signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]