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

[Dunrong Huang]

Hi, Marc-André

Thanks for reply

2012/11/23 Marc-André Lureau <mlureau at redhat.com>:
> 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?
I made a simple research how virtualbox implement dnd on client side,
since only client source is opensource.
It's more complex than what we do.
You can take a simple look at in this page:
http://www.virtualbox.org/svn/vbox/trunk/include/VBox/HostServices/DragAndDropSvc.h
>
>> 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
Oop, It's a typo.
>
> If we have to include a archive/compression I would go for tar.xz directly. I think compression could be added later.
Actually, compressed_format message has not been used in current code,
I defined it for future work here.
But you are right, it should 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?
If is_dir is true, just  create the directory.

For example:
A dir named "doc" has two files a.txt and b.txt.

First we send a start message with file_name = "doc" then message with
file_name = "doc/a.txt" and message with file_name = "doc/b.txt" will
be  sent.

So guest can create a direcory doc, and doc/a.txt, doc/b.txt.

In this way, we can implement transfer directory.
>
>> +   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!



-- 
Best Regards,

Dunrong Huang