| Message ID | 20220920103159.1865256-26-bmeng.cn@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | tests/qtest: Enable running qtest on Windows | expand |
Hi On Tue, Sep 20, 2022 at 2:33 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. > > As per [1] we should add FILE_SHARE_WRITE to the share mode to allow > such use case. This change makes the behavior be consistent with the > POSIX platforms. > > [1] > https://docs.microsoft.com/en-us/windows/win32/fileio/creating-and-opening-files > > Signed-off-by: Xuzhou Cheng <xuzhou.cheng@windriver.com> > Signed-off-by: Bin Meng <bin.meng@windriver.com> > --- > > Changes in v2: > - Update commit message to include the use case why we should set > FILE_SHARE_WRITE when openning the file for win32 > As discussed in v1, I would rather leave that patch out of this series, since the visible issue is solved differently elsewhere. Also, I disagree with the approach to make windows behaviour consistent with posix here, since I consider the windows behaviour (exclusive write access) superior. I would rather teach the posix implementation about exclusive write access. > chardev/char-file.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/chardev/char-file.c b/chardev/char-file.c > index 2fd80707e5..66385211eb 100644 > --- a/chardev/char-file.c > +++ b/chardev/char-file.c > @@ -60,8 +60,8 @@ static void qmp_chardev_open_file(Chardev *chr, > flags = CREATE_ALWAYS; > } > > - out = CreateFile(file->out, accessmode, FILE_SHARE_READ, NULL, flags, > - FILE_ATTRIBUTE_NORMAL, NULL); > + out = CreateFile(file->out, accessmode, FILE_SHARE_READ | > FILE_SHARE_WRITE, > + NULL, flags, FILE_ATTRIBUTE_NORMAL, NULL); > if (out == INVALID_HANDLE_VALUE) { > error_setg(errp, "open %s failed", file->out); > return; > -- > 2.34.1 > > >
On Fri, Sep 23, 2022 at 4:09 AM Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > > Hi > > On Tue, Sep 20, 2022 at 2:33 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. >> >> As per [1] we should add FILE_SHARE_WRITE to the share mode to allow >> such use case. This change makes the behavior be consistent with the >> POSIX platforms. >> >> [1] https://docs.microsoft.com/en-us/windows/win32/fileio/creating-and-opening-files >> >> Signed-off-by: Xuzhou Cheng <xuzhou.cheng@windriver.com> >> Signed-off-by: Bin Meng <bin.meng@windriver.com> >> --- >> >> Changes in v2: >> - Update commit message to include the use case why we should set >> FILE_SHARE_WRITE when openning the file for win32 > > > As discussed in v1, I would rather leave that patch out of this series, since the visible issue is solved differently elsewhere. > > Also, I disagree with the approach to make windows behaviour consistent with posix here, since I consider the windows behaviour (exclusive write access) superior. I would rather teach the posix implementation about exclusive write access. > Okay, will drop this patch in v3. Regards, Bin
On Sat, Sep 24, 2022 at 4:10 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > On Fri, Sep 23, 2022 at 4:09 AM Marc-André Lureau > <marcandre.lureau@gmail.com> wrote: > > > > Hi > > > > On Tue, Sep 20, 2022 at 2:33 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. > >> > >> As per [1] we should add FILE_SHARE_WRITE to the share mode to allow > >> such use case. This change makes the behavior be consistent with the > >> POSIX platforms. > >> > >> [1] https://docs.microsoft.com/en-us/windows/win32/fileio/creating-and-opening-files > >> > >> Signed-off-by: Xuzhou Cheng <xuzhou.cheng@windriver.com> > >> Signed-off-by: Bin Meng <bin.meng@windriver.com> > >> --- > >> > >> Changes in v2: > >> - Update commit message to include the use case why we should set > >> FILE_SHARE_WRITE when openning the file for win32 > > > > > > As discussed in v1, I would rather leave that patch out of this series, since the visible issue is solved differently elsewhere. > > > > Also, I disagree with the approach to make windows behaviour consistent with posix here, since I consider the windows behaviour (exclusive write access) superior. I would rather teach the posix implementation about exclusive write access. > > > > Okay, will drop this patch in v3. > Oops, I drew the conclusion too early. Actually there is another test case that needs this patch, which is boot-serial-test. Log below: # starting QEMU: ./qemu-system-x86_64 -qtest unix:G:\msys64\tmp/qtest-37112.sock -qtest-log nul -chardev socket,path=G:\msys64\tmp/qtest-37112.qmp,id=char0 -mon chardev=char0,mode=control -display none -M isapc -no-shutdown -chardev file,id=serial0, path=G:\msys64\tmp\qtest-boot-serial-sHH9FS1 -serial chardev:serial0 -accel tcg -accel kvm -cpu qemu32 -M graphics=off -accel qtest qemu-system-x86_64: -chardev file,id=serial0,path=G:\msys64\tmp\qtest-boot-serial-sHH9FS1: open G:\msys64\tmp\qtest-boot-serial-sHH9FS1 failed The serial chardev file was created by the qtest executable with g_file_open_tmp(), and later opened by the QEMU executable. g_file_open_tmp() opens the file with FILE_SHARE_WRITE, see g_open() for the details. Then based on https://learn.microsoft.com/en-us/windows/win32/fileio/creating-and-opening-files, there is only one case that first call to CreateFile with GENERIC_READ, FILE_SHARE_WRITE, and second call to CreateFile with GENERIC_WRITE, FILE_SHARE_READ is allowed. Other combinations all 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 flags were passed to the first call, so we will have to add FILE_SHARE_WRITE to the second call. I will add more details in the commit message in v3. Regards, Bin
diff --git a/chardev/char-file.c b/chardev/char-file.c index 2fd80707e5..66385211eb 100644 --- a/chardev/char-file.c +++ b/chardev/char-file.c @@ -60,8 +60,8 @@ static void qmp_chardev_open_file(Chardev *chr, flags = CREATE_ALWAYS; } - out = CreateFile(file->out, accessmode, FILE_SHARE_READ, NULL, flags, - FILE_ATTRIBUTE_NORMAL, NULL); + out = CreateFile(file->out, accessmode, FILE_SHARE_READ | FILE_SHARE_WRITE, + NULL, flags, FILE_ATTRIBUTE_NORMAL, NULL); if (out == INVALID_HANDLE_VALUE) { error_setg(errp, "open %s failed", file->out); return;