1. Patchew
  2. QEMU
  3. [PATCH 00/51] tests/qtest: Enable running qtest on Windows
  4. View patch

[PATCH 40/51] chardev/char-file: Add FILE_SHARE_WRITE when openning the file for win32

Bin Meng posted 51 patches 3 years, 5 months ago
Maintainers: "Alex Bennée" <alex.bennee@linaro.org>, "Philippe Mathieu-Daudé" <f4bug@amsat.org>, Thomas Huth <thuth@redhat.com>, Wainer dos Santos Moschetta <wainersm@redhat.com>, Beraldo Leal <bleal@redhat.com>, Laurent Vivier <lvivier@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Stefan Berger <stefanb@linux.vnet.ibm.com>, Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Greg Kurz <groug@kaod.org>, Christian Schoenebeck <qemu_oss@crudebyte.com>, "Cédric Le Goater" <clg@kaod.org>, Daniel Henrique Barboza <danielhb413@gmail.com>, David Gibson <david@gibson.dropbear.id.au>, Gerd Hoffmann <kraxel@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Yanan Wang <wangyanan55@huawei.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Michael Roth <michael.roth@amd.com>, Konstantin Kostiuk <kkostiuk@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Juan Quintela <quintela@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, John Snow <jsnow@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, Andrew Jeffery <andrew@aj.id.au>, Joel Stanley <joel@jms.id.au>, "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <ani@anisinha.ca>, Alexander Bulekov <alxndr@bu.edu>, Bandan Das <bsd@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Darren Kenny <darren.kenny@oracle.com>, Qiuhao Li <Qiuhao.Li@outlook.com>, Havard Skinnemoen <hskinnemoen@google.com>, Tyrone Ting <kfting@nuvoton.com>, Markus Armbruster <armbru@redhat.com>, Coiby Xu <Coiby.Xu@gmail.com>, Jason Wang <jasowang@redhat.com>, Fam Zheng <fam@euphon.net>
There is a newer version of this series
[PATCH 40/51] chardev/char-file: Add FILE_SHARE_WRITE when openning the file for win32
Posted by Bin Meng 3 years, 5 months ago 
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 openned.

As per [1] we should add FILE_SHARE_WRITE to the share mode to allow
such use case. This change makes the behavior be consisten 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>
---

 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
Re: [PATCH 40/51] chardev/char-file: Add FILE_SHARE_WRITE when openning the file for win32
Posted by Marc-André Lureau 3 years, 5 months ago 
Hi

On Wed, Aug 24, 2022 at 1:43 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 openned.

opened

>
> As per [1] we should add FILE_SHARE_WRITE to the share mode to allow
> such use case. This change makes the behavior be consisten with the
> POSIX platforms.
>

consistent

> [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>
> ---


What's the benefit to allow multiple processes write access to the
same file? It seems it could easily lead to corruption or unexpected
results.

To me, it's the other way around, the POSIX implementation should
learn to lock the file opened for write..

>
>  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
>
Re: [PATCH 40/51] chardev/char-file: Add FILE_SHARE_WRITE when openning the file for win32
Posted by Bin Meng 3 years, 5 months ago 
On Thu, Aug 25, 2022 at 3:59 PM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Hi
>
> On Wed, Aug 24, 2022 at 1:43 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 openned.
>
> opened
>
> >
> > As per [1] we should add FILE_SHARE_WRITE to the share mode to allow
> > such use case. This change makes the behavior be consisten with the
> > POSIX platforms.
> >
>
> consistent
>
> > [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>
> > ---
>
>
> What's the benefit to allow multiple processes write access to the
> same file? It seems it could easily lead to corruption or unexpected
> results.

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 new one still holds the file handle that does
not allow shared write permission then the new QEMU process will fail.

> To me, it's the other way around, the POSIX implementation should
> learn to lock the file opened for write..
>

Regards,
Bin
Re: [PATCH 40/51] chardev/char-file: Add FILE_SHARE_WRITE when openning the file for win32
Posted by Marc-André Lureau 3 years, 5 months ago 
Hi

On Fri, Aug 26, 2022 at 5:16 PM Bin Meng <bmeng.cn@gmail.com> wrote:

> On Thu, Aug 25, 2022 at 3:59 PM Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
> >
> > Hi
> >
> > On Wed, Aug 24, 2022 at 1:43 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 openned.
> >
> > opened
> >
> > >
> > > As per [1] we should add FILE_SHARE_WRITE to the share mode to allow
> > > such use case. This change makes the behavior be consisten with the
> > > POSIX platforms.
> > >
> >
> > consistent
> >
> > > [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>
> > > ---
> >
> >
> > What's the benefit to allow multiple processes write access to the
> > same file? It seems it could easily lead to corruption or unexpected
> > results.
>
> 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 new one still holds the file handle that does
> not allow shared write permission then the new QEMU process will fail.
>
>
Thanks for the details, that's worth to add in commit message imho.

But can't we fix the test instead to use different paths?

-- 
Marc-André Lureau
Re: [PATCH 40/51] chardev/char-file: Add FILE_SHARE_WRITE when openning the file for win32
Posted by Bin Meng 3 years, 5 months ago 
On Fri, Aug 26, 2022 at 9:23 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Fri, Aug 26, 2022 at 5:16 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>
>> On Thu, Aug 25, 2022 at 3:59 PM Marc-André Lureau
>> <marcandre.lureau@redhat.com> wrote:
>> >
>> > Hi
>> >
>> > On Wed, Aug 24, 2022 at 1:43 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 openned.
>> >
>> > opened
>> >
>> > >
>> > > As per [1] we should add FILE_SHARE_WRITE to the share mode to allow
>> > > such use case. This change makes the behavior be consisten with the
>> > > POSIX platforms.
>> > >
>> >
>> > consistent
>> >
>> > > [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>
>> > > ---
>> >
>> >
>> > What's the benefit to allow multiple processes write access to the
>> > same file? It seems it could easily lead to corruption or unexpected
>> > results.
>>
>> 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 new one still holds the file handle that does
>> not allow shared write permission then the new QEMU process will fail.
>>
>
> Thanks for the details, that's worth to add in commit message imho.

Sure, I can add this in the commit message.

>
> But can't we fix the test instead to use different paths?
>

Yeah, the test case fix is here:
https://lore.kernel.org/qemu-devel/20220824094029.1634519-42-bmeng.cn@gmail.com/

I think this patch is still needed as it makes the Windows char-file
behavior be consistent with the posix because there is no lock
mechanism in posix.

Regards,
Bin
Re: [PATCH 40/51] chardev/char-file: Add FILE_SHARE_WRITE when openning the file for win32
Posted by Marc-André Lureau 3 years, 5 months ago 
Hi

On Sun, Aug 28, 2022 at 3:19 AM Bin Meng <bmeng.cn@gmail.com> wrote:

> On Fri, Aug 26, 2022 at 9:23 PM Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> >
> > Hi
> >
> > On Fri, Aug 26, 2022 at 5:16 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >>
> >> On Thu, Aug 25, 2022 at 3:59 PM Marc-André Lureau
> >> <marcandre.lureau@redhat.com> wrote:
> >> >
> >> > Hi
> >> >
> >> > On Wed, Aug 24, 2022 at 1:43 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 openned.
> >> >
> >> > opened
> >> >
> >> > >
> >> > > As per [1] we should add FILE_SHARE_WRITE to the share mode to allow
> >> > > such use case. This change makes the behavior be consisten with the
> >> > > POSIX platforms.
> >> > >
> >> >
> >> > consistent
> >> >
> >> > > [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>
> >> > > ---
> >> >
> >> >
> >> > What's the benefit to allow multiple processes write access to the
> >> > same file? It seems it could easily lead to corruption or unexpected
> >> > results.
> >>
> >> 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 new one still holds the file handle that does
> >> not allow shared write permission then the new QEMU process will fail.
> >>
> >
> > Thanks for the details, that's worth to add in commit message imho.
>
> Sure, I can add this in the commit message.
>
> >
> > But can't we fix the test instead to use different paths?
> >
>
> Yeah, the test case fix is here:
>
> https://lore.kernel.org/qemu-devel/20220824094029.1634519-42-bmeng.cn@gmail.com/
>
> I think this patch is still needed as it makes the Windows char-file
> behavior be consistent with the posix because there is no lock
> mechanism in posix.
>

In this case, I would rather make posix consistent with the windows
behaviour :)

I am not sure how to proceed from there, but I would discard your windows
patch for now.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.