Re: [PATCH v3 1/1] hv_fcopy_uio_daemon: Fix file copy failure between Windows host and Linux guest

[Yasumasa Suenaga <yasuenag@gmail.com>]

Hi Naman,
Sorry for bothering you, but I have some questions:


> 1. Create a new patch file for every new version and then send it.
> Currently it seems you are manually editing the same patch file in the
> subject and sending it, so each patch version is showing up in the same
> thread.

I've committed changes with "git amend" and created formatted patch with "git format-patch",
then I sent all of *.patch files via "git send-email".
Do you mean I should commit to fix commits reviewers and send cover letter and incremental
patches only? I couldn't find about it from the guide.
I checked linux-hyperv list, all of patches have been sent in each version AFAICS.


> 3. Keep a minimum of 1-2 weeks gap between successive patch versions to
> give time to people to review your changes.

Does it include changes of commit message too?


> 4. In the commit msg of v3, it is still not very clear what problem, you
> are trying to fix here. Do you mean to say that fcopy does not work on
> Linux? Or you are assuming it won't work and fixing some generic
> problem?
> Fcopy is supposed to work fine on Linux VM on HyperV with windows host. If there are some errors, please share in cover letter/comments
> in the patch along with steps of execution.

I have a problem on my PC:

   Host: Windows 11 Pro (24H2 build 26100.4202)
   Guest: Fedora 42
     - kernel-6.14.4-300.fc42.x86_64
     - hypervfcopyd-6.10-1.fc42.x86_64

How to reproduce: run Copy-VMFile commandlet on Host:

Following log is in Japanese because my PC is set to Japanese, sorry.
But it says fcopy could not transfer file (test.ps1) to /tmp/ on Linux guest because it already exists.
I confirmed /tmp/test.ps1 does not exist of course.

```
> Copy-VMFile

cmdlet Copy-VMFile at command pipeline position 1
Supply values for the following parameters:
Name[0]: Fedora42
Name[1]:
SourcePath: test.ps1
DestinationPath: /tmp/
FileSource: Host
Copy-VMFile: ゲストへのファイルのコピーを開始できませんでした。

ソース ファイル 'C:\test\test.ps1' をゲストの宛先 '/tmp/' にコピーできませんでした。

'Fedora42' はゲスト: ファイルがあります。 (0x80070050) へのファイルのコピーを開始できませんでした。(仮想マシン ID 9BFDF23D-CCAA-4748-A770-6D654E09A133)

'Fedora42' は、コピー元ファイル 'C:\test\test.ps1' をゲスト上のコピー先 '/tmp/' にコピーできませんでした: ファイルがあります。 (0x80070050)。(仮想マシン ID 9BFDF23D-CCAA-4748-A770-6D654E09A133)
```

I got following fcopy log from journald - it is strange because "/tmp/test.ps1" should be shown here.
```
6月 04 17:48:24 fc42 HV_UIO_FCOPY[1080]: File: / exists
```

As I wrote in commit message, wchar_t is 32 bit in Linux. I confirmed it with "sizeof(wchar_t)".
However fcopyd handles it as 16 bit value (__u16), thus I think this is a bug in fcopy, and
I think it would also not work on other environments.

Actually it works fine with my patch to handle values as 16 bit.


Thanks,

Yasumasa


On 2025/06/04 15:19, Naman Jain wrote:
> 
> 
> On 6/4/2025 5:13 AM, yasuenag@gmail.com wrote:
>> From: Yasumasa Suenaga <yasuenag@gmail.com>
>>
>> Handle file copy request from the host (e.g. Copy-VMFile commandlet)
>> correctly.
>> Store file path and name as __u16 arrays in struct hv_start_fcopy.
>> Convert directly to UTF-8 string without casting to wchar_t* in fcopyd.
>>
>> Fix string conversion failure caused by wchar_t size difference between
>> Linux (32bit) and Windows (16bit). Convert each character to char
>> if the value is less than 0x80 instead of using wcstombs() call.
>>
>> Add new check to snprintf() call for target path creation to handle
>> length differences between PATH_MAX (Linux) and W_MAX_PATH (Windows).
>>
>> Signed-off-by: Yasumasa Suenaga <yasuenag@gmail.com>
>> ---
>>   tools/hv/hv_fcopy_uio_daemon.c | 37 ++++++++++++++--------------------
>>   1 file changed, 15 insertions(+), 22 deletions(-)
>>
>> diff --git a/tools/hv/hv_fcopy_uio_daemon.c b/tools/hv/hv_fcopy_uio_daemon.c
>> index 0198321d1..86702f39e 100644
>> --- a/tools/hv/hv_fcopy_uio_daemon.c
>> +++ b/tools/hv/hv_fcopy_uio_daemon.c
>> @@ -62,8 +62,11 @@ static int hv_fcopy_create_file(char *file_name, char *path_name, __u32 flags)
>>       filesize = 0;
>>       p = path_name;
>> -    snprintf(target_fname, sizeof(target_fname), "%s/%s",
>> -         path_name, file_name);
>> +    if (snprintf(target_fname, sizeof(target_fname), "%s/%s",
>> +             path_name, file_name) >= sizeof(target_fname)) {
>> +        /* target file name is too long */
>> +        goto done;
>> +    }
>>       /*
>>        * Check to see if the path is already in place; if not,
>> @@ -273,6 +276,8 @@ static void wcstoutf8(char *dest, const __u16 *src, size_t dest_size)
>>       while (len < dest_size) {
>>           if (src[len] < 0x80)
>>               dest[len++] = (char)(*src++);
>> +        else if (src[len] == '0')
>> +            break;
>>           else
>>               dest[len++] = 'X';
>>       }
>> @@ -282,27 +287,15 @@ static void wcstoutf8(char *dest, const __u16 *src, size_t dest_size)
>>   static int hv_fcopy_start(struct hv_start_fcopy *smsg_in)
>>   {
>> -    setlocale(LC_ALL, "en_US.utf8");
>> -    size_t file_size, path_size;
>> -    char *file_name, *path_name;
>> -    char *in_file_name = (char *)smsg_in->file_name;
>> -    char *in_path_name = (char *)smsg_in->path_name;
>> -
>> -    file_size = wcstombs(NULL, (const wchar_t *restrict)in_file_name, 0) + 1;
>> -    path_size = wcstombs(NULL, (const wchar_t *restrict)in_path_name, 0) + 1;
>> -
>> -    file_name = (char *)malloc(file_size * sizeof(char));
>> -    path_name = (char *)malloc(path_size * sizeof(char));
>> -
>> -    if (!file_name || !path_name) {
>> -        free(file_name);
>> -        free(path_name);
>> -        syslog(LOG_ERR, "Can't allocate memory for file name and/or path name");
>> -        return HV_E_FAIL;
>> -    }
>> +    /*
>> +     * file_name and path_name should have same length with appropriate
>> +     * member of hv_start_fcopy.
>> +     */
>> +    char file_name[W_MAX_PATH], path_name[W_MAX_PATH];
>> -    wcstoutf8(file_name, (__u16 *)in_file_name, file_size);
>> -    wcstoutf8(path_name, (__u16 *)in_path_name, path_size);
>> +    setlocale(LC_ALL, "en_US.utf8");
>> +    wcstoutf8(file_name, smsg_in->file_name, W_MAX_PATH - 1);
>> +    wcstoutf8(path_name, smsg_in->path_name, W_MAX_PATH - 1);
>>       return hv_fcopy_create_file(file_name, path_name, smsg_in->copy_flags);
>>   }
> 
> Hi,
> I understand this is your first patch for upstreaming. Here are a few
> things you should consider:
> 1. Create a new patch file for every new version and then send it.
> Currently it seems you are manually editing the same patch file in the
> subject and sending it, so each patch version is showing up in the same
> thread.
> 2. Read, re-read, absorb the information in the link that Easwar also
> mentioned:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> 3. Keep a minimum of 1-2 weeks gap between successive patch versions to
> give time to people to review your changes.
> 4. In the commit msg of v3, it is still not very clear what problem, you
> are trying to fix here. Do you mean to say that fcopy does not work on
> Linux? Or you are assuming it won't work and fixing some generic
> problem?
> Fcopy is supposed to work fine on Linux VM on HyperV with windows host. If there are some errors, please share in cover letter/comments
> in the patch along with steps of execution.
> 
> 5. If its a fix, we should have a proper Fixes tag with the commit you
> are fixing.
> 6. Have a look at existing conversations at lore to get to know common
> practices with single patch, multi patch, cover letters etc.
> https://lore.kernel.org/linux-hyperv/?q=linux-hyperv
> 
> Regards,
> Naman
> 
>