Re: [PATCH v3 42/54] chardev/char-file: Add FILE_SHARE_WRITE when opening the file for win32

[Bin Meng]

On Tue, Sep 27, 2022 at 5:00 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Mon, Sep 26, 2022 at 7:05 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>
>> On Mon, Sep 26, 2022 at 9:27 PM Marc-André Lureau
>> <marcandre.lureau@gmail.com> wrote:
>> >
>> > Hi
>> >
>> > On Sun, Sep 25, 2022 at 4:35 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>> >>
>> >> From: Xuzhou Cheng <xuzhou.cheng@windriver.com>
>> >>
>> >> The combination of GENERIC_WRITE and FILE_SHARE_READ options does not
>> >> allow the same file to be opened again by CreateFile() from another
>> >> QEMU process with the same options when the previous QEMU process
>> >> still holds the file handle opened.
>> >>
>> >> This was triggered by running the test_multifd_tcp_cancel() case on
>> >> Windows, which cancels the migration, and launches another QEMU
>> >> process to migrate with the same file opened for write. Chances are
>> >> that the previous QEMU process does not quit before the new QEMU
>> >> process runs hence the old one still holds the file handle that does
>> >> not allow shared write permission then the new QEMU process will fail.
>> >>
>> >> There is another test case boot-serial-test that triggers the same
>> >> issue. The qtest executable created a serial chardev file to be
>> >> passed to the QEMU executable. The serial file was created by
>> >> g_file_open_tmp(), which internally opens the file with
>> >> FILE_SHARE_WRITE security attribute, and based on [1], there is
>> >> only one case that allows the first call to CreateFile() with
>> >> GENERIC_READ & FILE_SHARE_WRITE, and second call to CreateFile()
>> >> with GENERIC_WRITE & FILE_SHARE_READ. All other combinations
>> >> require FILE_SHARE_WRITE in the second call. But there is no way
>> >> for the second call (in this case the QEMU executable) to know
>> >> what combination was passed to the first call, so we will have to
>> >> add FILE_SHARE_WRITE to the second call.
>> >>
>> >> For both scenarios we should add FILE_SHARE_WRITE in the chardev
>> >> file backend driver. This change also makes the behavior to be
>> >> consistent with the POSIX platforms.
>> >
>> >
>> > It seems to me the tests should be fixed instead. I thought you fixed the 
>> > first case. For the second case, why not close the file before starting 
>> > qemu? If you have issues, I will take a deeper look.
>>
>> Indeed, the following test case change can "fix" this issue:
>>
>> diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
>> index 72310ba30e..f192fbc181 100644
>> --- a/tests/qtest/boot-serial-test.c
>> +++ b/tests/qtest/boot-serial-test.c
>> @@ -233,6 +233,7 @@ static void test_machine(const void *data)
>> ser_fd = g_file_open_tmp("qtest-boot-serial-sXXXXXX", &serialtmp, NULL);
>> g_assert(ser_fd != -1);
>> + close(ser_fd);
>> if (test->kernel) {
>> code = test->kernel;
>> @@ -266,6 +267,7 @@ static void test_machine(const void *data)
>> unlink(codetmp);
>> }
>> + ser_fd = open(serialtmp, O_RDONLY);
>> if (!check_guest_output(qts, test, ser_fd)) {
>> g_error("Failed to find expected string. Please check '%s'",
>> serialtmp);
>>
>
> Please send this fix as a new patch in the series.
>
>>
>> But I think it just workarounds the problem. The original test case
>> looks reasonable to me. If we update the case like above, we cannot
>> guarantee users will do like the updated test case does.
>
>
> If the test is enabled, it will fail, and the reasons are reasonably valid: 
> two processes shouldn't share the same file for writing with a chardev.
>
> I still think the windows file chardev behavior is superior and we should 
> instead teach the posix implementation of exclusive write access, rather than 
> downgrading the windows implementation. I'd drop this patch from the series 
> for now.
>

Okay, will drop this patch, and add the test case fix as a new patch in v4.

Regards,
Bin