[Spice-devel] [PATCH spice-protocol V2] vd_agent.h: drag-and-drop support

[Marc-André Lureau]

Hi

----- Mensaje original -----
> At present, Vmware and Virtualbox has supported file drag&drop
> feature,
> I think it's a good feature for users, so we want qemu/spice to
> supports it.

Btw, have you done a small comparison of what VirtualBox can support and how it is implemented?

> This patch first adds communication protocol between client and
> guest,
> we must make the agent protocol stable before coding, this is what we
> want this patch to do.
> 
> This feature has been discussed on spice mailing list.
> 
> The more details are available at following pages:
> http://lists.freedesktop.org/archives/spice-devel/2012-November/011400.html
> and
> http://lists.freedesktop.org/archives/spice-devel/2012-November/011485.html
> 
> Signed-off-by: Dunrong Huang <riegamaths at gmail.com>
> Cc: Hans de Goede <hdegoede at redhat.com>
> Cc: Marc-André Lureau <marcandre.lureau at redhat.com>
> Cc: Alon Levy <alevy at redhat.com>
> Cc: Uri Lublin <uril at redhat.com>
> ---
>  spice/vd_agent.h | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> index 2b9884e..f6cc49e 100644
> --- a/spice/vd_agent.h
> +++ b/spice/vd_agent.h
> @@ -69,9 +69,44 @@ enum {
>      VD_AGENT_CLIPBOARD_GRAB,
>      VD_AGENT_CLIPBOARD_REQUEST,
>      VD_AGENT_CLIPBOARD_RELEASE,
> +    VD_AGENT_FILE_XFER_START,
> +    VD_AGENT_FILE_XFER_STATUS,
> +    VD_AGENT_FILE_XFER_DATA,
>      VD_AGENT_END_MESSAGE,
>  };
>  
> +enum {
> +    VD_AGENT_FILE_XFER_RESULT_SUCCESS,

What does success indicates? When is it sent?

> +    VD_AGENT_FILE_XFER_RESULT_DISK_FULL,

What can the client do about it that the guest can't? I am not sure we need separate error for "disk full" situation. If we do, then I guess we need a lot more errors (invalid filename, file too long, exists, permission denied, read only...)

> +    VD_AGENT_FILE_XFER_RESULT_CANCELLED,

In addition (can be added later on), I think we could use PAUSE/RESUME.

> +    VD_AGENT_FILE_XFER_RESULT_ERROR,
> +};

Rather than "result", it looks like VD_AGENT_FILE_XFER_STATUS_ would fit better, since it is embedded the StatusMessage.

> +
> +typedef struct SPICE_ATTR_PACKED VDAgentFileXferStatusMessage {
> +   uint32_t id;
> +   uint32_t result;
> +} VDAgentFileXferStatusMessage;
> +
> +enum {
> +    VD_AGENT_FILE_XFER_FORMAT_RAW,
> +    VD_AGENT_FILE_XFER_RESULT_GZIP,
> +};

enum prefix XFER_FORMAT != XFER_RESULT

If we have to include a archive/compression I would go for tar.xz directly. I think compression could be added later.

> +
> +typedef struct SPICE_ATTR_PACKED VDAgentFileXferStartMessage {
> +   uint32_t id;
> +   uint32_t is_dir;

What if is_dir is true? How do you know each file name & size then? Or you send a separate message just for the directory creation?

> +   uint32_t compressed_format;
> +   uint32_t file_mode;
> +   uint64_t file_size;

Just some thoughts:
If the file is compressed it will be hard to know in advance the file_size, unless we compress all before sending, which is a waste of resources. file_size could be the final decompress size. And after sending all the VDAgentFileXferDataMessage, the client could send a final VD_AGENT_FILE_XFER_RESULT_SUCCESS, to indicate eof?

> +   uint8_t file_name[0];
> +} VDAgentFileXferStartMessage;

Instead of those fields that will be hard to extend and that mostly only concern the guest agent, I would consider a key-value text such as:

[vdagent-file-xfer]
type=regular
mime=text/plain
name=foo.sh
time-modified=1353085649
time-...
can-read=true
can-execute=true
can-write=false
size=25

[vdagent-file-xfer]
type=directory
mime=application/x-xz-compressed-tar
name=Downloads
time-modified=1353085649
time-...
can-read=true
can-execute=true
can-write=false
size=515416
[vdagent-context]
pointer-position=+34+98

We need to define the mandatory group (vdagent-file-xfer) and keys (file, type, mime, size..). 

If qemu needs to apply some kind of filtering (which I think is really hard to do, you basically allow copy or not at all), some of the fields could be in the message itself eventually, to avoid qemu parsing that text file.

> +typedef struct SPICE_ATTR_PACKED VDAgentFileXferDataMessage {
> +   uint32_t id;
> +   uint64_t size;

uint64_t is certainly too large, and you can deduce data size from the message size.

> +   uint8_t data[0];
> +} VDAgentFileXferDataMessage;
> +

Data can be sent right away without waiting for status message? In which case, the client will need to queue or save in temp the received data, but why not.

Thanks again for your work!