Re: [PATCH win-vdagent v2] Support file transfer for existing files

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

 



On Fri, 2017-04-28 at 05:55 -0400, Frediano Ziglio wrote:
> > 
> > On Thu, 2017-04-27 at 15:20 -0500, Jonathon Jongsma wrote:
> > > Previously, if the user attempted to transfer a file that had the
> > > same
> > > name as a file that already exists on the guest, we would just
> > > fail
> > > the
> > > transfer. This patch tries to match the behavior of the linux
> > > vdagent
> > > where we try to append an integer onto the name of the new file
> > > to
> > > make
> > > it unique. For example, if you tried to transfer 'file.doc' to
> > > the
> > > guest
> > > and that file already existed, it would try to create 'file
> > > (1).doc'
> > > instead. If that also failed, it would attempt 'file (2).doc',
> > > etc,
> > > up
> > > to 63.
> > > 
> > > Resolves: rhbz#1410181
> > > Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> > > ---
> > > Changes since v1:
> > >  - change from "file.doc (1)" to "file (1).doc"
> > >  - retain debug output for other CreateFile errors
> > > 
> > >  vdagent/file_xfer.cpp | 50
> > > ++++++++++++++++++++++++++++++++++++++++++++------
> > >  1 file changed, 44 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/vdagent/file_xfer.cpp b/vdagent/file_xfer.cpp
> > > index f763ed3..24eb00a 100644
> > > --- a/vdagent/file_xfer.cpp
> > > +++ b/vdagent/file_xfer.cpp
> > > @@ -61,6 +61,7 @@ void
> > > FileXfer::handle_start(VDAgentFileXferStartMessage* start,
> > >      HANDLE handle;
> > >      AsUser as_user;
> > >      int wlen;
> > > +    int attempt = 0;
> > >  
> > >      status->id = start->id;
> > >      status->result = VD_AGENT_FILE_XFER_STATUS_ERROR;
> > > @@ -99,15 +100,52 @@ void
> > > FileXfer::handle_start(VDAgentFileXferStartMessage* start,
> > >  
> > >      file_path[wlen++] = TEXT('\\');
> > >      file_path[wlen] = TEXT('\0');
> > > -    if((wlen = MultiByteToWideChar(CP_UTF8, 0, file_name, -1,
> > > file_path + wlen, MAX_PATH - wlen)) == 0){
> > > -        vd_printf("failed converting file_name:%s to WideChar",
> > > file_name);
> > > -        return;
> > > +
> > > +    const int MAX_ATTEMPTS = 64; // matches behavior of linux
> > > vdagent
> > > +    const size_t POSTFIX_LEN = 6; // up to 2 digits in
> > > parentheses
> > > and final NULL: " (xx)"
> > > +    size_t name_len = strlen(file_name);
> > > +    char *extension = strrchr(file_name, '.');
> > > +
> > > +    for (attempt = 0; attempt < MAX_ATTEMPTS; attempt++) {
> > > +        char dest_filename[MAX_PATH];
> > > +        if (attempt == 0) {
> > > +            strcpy(dest_filename, file_name);
> > > +        } else {
> > > +            if (name_len + POSTFIX_LEN > MAX_PATH) {
> > > +                vd_printf("failed creating %ls. Postfix makes
> > > name
> > > too long for the buffer.", file_path);
> > > +                return;
> > > +            }
> > > +
> > > +            int basename_len = (extension != NULL) ? (extension
> > > -
> > > file_name) : name_len;
> > > +            memcpy(dest_filename, file_name, basename_len);
> > > +            // append postfix
> > > +            int postfix_len = snprintf(dest_filename +
> > > basename_len, POSTFIX_LEN, " (%d)", attempt);
> > > +            if (extension != NULL) {
> > > +                strcpy(dest_filename + basename_len +
> > > postfix_len,
> > > extension);
> > > +            }
> > > +        }
> > > +        if((MultiByteToWideChar(CP_UTF8, 0, dest_filename, -1,
> > 
> > missing space^
> > > file_path + wlen, MAX_PATH - wlen)) == 0){
> > 
> >                                   and here ^
> > > +            vd_printf("failed converting file_name:%s to
> > > WideChar",
> > > dest_filename);
> > > +            return;
> > > +        }
> > > +        handle = CreateFile(file_path, GENERIC_WRITE, 0, NULL,
> > > CREATE_NEW, 0, NULL);
> > > +        if (handle != INVALID_HANDLE_VALUE) {
> > > +            break;
> > > +        }
> > > +
> > > +        // If the file already exists, we can re-try with a new
> > > filename. If
> > > +        // it's a different error, there's not much we can do.
> > > +        if (GetLastError() != ERROR_FILE_EXISTS) {
> > > +            vd_printf("Failed creating %ls %lu", file_path,
> > > GetLastError());
> > > +            return;
> > > +        }
> > >      }
> > > -    handle = CreateFile(file_path, GENERIC_WRITE, 0, NULL,
> > > CREATE_NEW, 0, NULL);
> > > -    if (handle == INVALID_HANDLE_VALUE) {
> > > -        vd_printf("failed creating %ls %lu", file_path,
> > > GetLastError());
> > > +
> > > +    if (attempt == MAX_ATTEMPTS) {
> > > +        vd_printf("Failed creating %ls. More than 63 copies
> > > exist?", file_path);
> > >          return;
> > >      }
> > > +
> > >      task = new FileXferTask(handle, file_size, file_path);
> > >      _tasks[start->id] = task;
> > >      status->result = VD_AGENT_FILE_XFER_STATUS_CAN_SEND_DATA;
> > 
> > looks good, ack
> > 
> > Pavel
> > 
> 
> I would propose a version like
> 
> 
> diff --git a/vdagent/file_xfer.cpp b/vdagent/file_xfer.cpp
> index f763ed3..de1aea1 100644
> --- a/vdagent/file_xfer.cpp
> +++ b/vdagent/file_xfer.cpp
> @@ -28,6 +28,8 @@
>  #endif // compiler specific definitions
>  
>  #include <stdio.h>
> +#include <spice/macros.h>
> +
>  #include "file_xfer.h"
>  #include "as_user.h"
>  
> @@ -99,13 +101,40 @@ void
> FileXfer::handle_start(VDAgentFileXferStartMessage* start,
>  
>      file_path[wlen++] = TEXT('\\');
>      file_path[wlen] = TEXT('\0');
> -    if((wlen = MultiByteToWideChar(CP_UTF8, 0, file_name, -1,
> file_path + wlen, MAX_PATH - wlen)) == 0){
> -        vd_printf("failed converting file_name:%s to WideChar",
> file_name);
> -        return;
> +
> +    const int MAX_ATTEMPTS = 64; // matches behavior of linux
> vdagent
> +    const size_t POSTFIX_LEN = 6; // up to 2 digits in parentheses
> and final NULL: " (xx)"
> +    char *extension = strrchr(file_name, '.');
> +    if (!extension)
> +        extension = strchr(file_name, 0);
> +
> +    for (int attempt = 0; attempt < MAX_ATTEMPTS; attempt++) {
> +        char dest_filename[SPICE_N_ELEMENTS(file_name) +
> POSTFIX_LEN];
> +        if (attempt == 0) {
> +            strcpy(dest_filename, file_name);
> +        } else {
> +            sprintf(dest_filename, "%.*s (%d)%s", int(extension -
> file_name), file_name,
> +                    attempt, extension);
> +        }
> +        if ((MultiByteToWideChar(CP_UTF8, 0, dest_filename, -1,
> file_path + wlen, MAX_PATH - wlen)) == 0) {
> +            vd_printf("failed converting file_name:%s to WideChar",
> dest_filename);
> +            return;
> +        }
> +        handle = CreateFile(file_path, GENERIC_WRITE, 0, NULL,
> CREATE_NEW, 0, NULL);
> +        if (handle != INVALID_HANDLE_VALUE) {
> +            break;
> +        }
> +
> +        // If the file already exists, we can re-try with a new
> filename. If
> +        // it's a different error, there's not much we can do.
> +        if (GetLastError() != ERROR_FILE_EXISTS) {
> +            vd_printf("Failed creating %ls %lu", file_path,
> GetLastError());
> +            return;
> +        }
>      }
> -    handle = CreateFile(file_path, GENERIC_WRITE, 0, NULL,
> CREATE_NEW, 0, NULL);
> +
>      if (handle == INVALID_HANDLE_VALUE) {
> -        vd_printf("failed creating %ls %lu", file_path,
> GetLastError());
> +        vd_printf("Failed creating %ls. More than 63 copies exist?",
> file_path);
>          return;
>      }
>      task = new FileXferTask(handle, file_size, file_path);
> 
> 
> is more similar to the way is proposed in Unix version of the agent


I agree that this is a nicer approach and is more consistent with the
linux agent. ACK

Jonathon

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]