[PATCH v3] block/file-win32: add reopen handlers

[PATCH v3] block/file-win32: add reopen handlers

[Viktor Prutyanov]

Make 'qemu-img commit' work on Windows.

Command 'commit' requires reopening backing file in RW mode. So,
add reopen prepare/commit/abort handlers and change dwShareMode
for CreateFile call in order to allow further read/write reopening.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/418
Suggested-by: Hanna Reitz <hreitz@redhat.com>
Signed-off-by: Viktor Prutyanov <viktor.prutyanov@phystech.edu>
---
 v2:
    - fix indentation in raw_reopen_prepare
    - free rs if raw_reopen_prepare fails
 v3:
    - restore suggested-by field missed in v2

 block/file-win32.c | 90 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 89 insertions(+), 1 deletion(-)

diff --git a/block/file-win32.c b/block/file-win32.c
index 2642088bd6..9dcbb2b53b 100644
--- a/block/file-win32.c
+++ b/block/file-win32.c
@@ -58,6 +58,10 @@ typedef struct BDRVRawState {
     QEMUWin32AIOState *aio;
 } BDRVRawState;
 
+typedef struct BDRVRawReopenState {
+    HANDLE hfile;
+} BDRVRawReopenState;
+
 /*
  * Read/writes the data to/from a given linear buffer.
  *
@@ -392,7 +396,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     s->hfile = CreateFile(filename, access_flags,
-                          FILE_SHARE_READ, NULL,
+                          FILE_SHARE_READ | FILE_SHARE_WRITE, NULL,
                           OPEN_EXISTING, overlapped, NULL);
     if (s->hfile == INVALID_HANDLE_VALUE) {
         int err = GetLastError();
@@ -634,6 +638,86 @@ static int coroutine_fn raw_co_create_opts(BlockDriver *drv,
     return raw_co_create(&options, errp);
 }
 
+static int raw_reopen_prepare(BDRVReopenState *state,
+                              BlockReopenQueue *queue, Error **errp)
+{
+    BDRVRawState *s = state->bs->opaque;
+    BDRVRawReopenState *rs;
+    int access_flags;
+    DWORD overlapped;
+    int ret = 0;
+
+    rs = g_new0(BDRVRawReopenState, 1);
+
+    raw_parse_flags(state->flags, s->aio != NULL, &access_flags, &overlapped);
+    rs->hfile = CreateFile(state->bs->filename, access_flags,
+                           FILE_SHARE_READ | FILE_SHARE_WRITE, NULL,
+                           OPEN_EXISTING, overlapped, NULL);
+
+    if (rs->hfile == INVALID_HANDLE_VALUE) {
+        int err = GetLastError();
+
+        error_setg_win32(errp, err, "Could not reopen '%s'",
+                         state->bs->filename);
+        if (err == ERROR_ACCESS_DENIED) {
+            ret = -EACCES;
+        } else {
+            ret = -EINVAL;
+        }
+        goto fail;
+    }
+
+    if (s->aio) {
+        ret = win32_aio_attach(s->aio, rs->hfile);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "Could not enable AIO");
+            goto fail;
+        }
+    }
+
+    state->opaque = rs;
+
+    return 0;
+
+fail:
+    g_free(rs);
+    state->opaque = NULL;
+
+    return ret;
+}
+
+static void raw_reopen_commit(BDRVReopenState *state)
+{
+    BDRVRawState *s = state->bs->opaque;
+    BDRVRawReopenState *rs = state->opaque;
+
+    if (!rs) {
+        return;
+    }
+
+    CloseHandle(s->hfile);
+    s->hfile = rs->hfile;
+
+    g_free(rs);
+    state->opaque = NULL;
+}
+
+static void raw_reopen_abort(BDRVReopenState *state)
+{
+    BDRVRawReopenState *rs = state->opaque;
+
+    if (!rs) {
+        return;
+    }
+
+    if (rs->hfile != INVALID_HANDLE_VALUE) {
+        CloseHandle(rs->hfile);
+    }
+
+    g_free(rs);
+    state->opaque = NULL;
+}
+
 static QemuOptsList raw_create_opts = {
     .name = "raw-create-opts",
     .head = QTAILQ_HEAD_INITIALIZER(raw_create_opts.head),
@@ -659,6 +743,10 @@ BlockDriver bdrv_file = {
     .bdrv_co_create_opts = raw_co_create_opts,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
 
+    .bdrv_reopen_prepare = raw_reopen_prepare,
+    .bdrv_reopen_commit  = raw_reopen_commit,
+    .bdrv_reopen_abort   = raw_reopen_abort,
+
     .bdrv_aio_preadv    = raw_aio_preadv,
     .bdrv_aio_pwritev   = raw_aio_pwritev,
     .bdrv_aio_flush     = raw_aio_flush,
-- 
2.21.0

Re: [PATCH v3] block/file-win32: add reopen handlers

[Philippe Mathieu-Daudé]

On 8/17/21 10:21 PM, Viktor Prutyanov wrote:
> Make 'qemu-img commit' work on Windows.
> 
> Command 'commit' requires reopening backing file in RW mode. So,
> add reopen prepare/commit/abort handlers and change dwShareMode
> for CreateFile call in order to allow further read/write reopening.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/418
> Suggested-by: Hanna Reitz <hreitz@redhat.com>
> Signed-off-by: Viktor Prutyanov <viktor.prutyanov@phystech.edu>
> ---
>  v2:
>     - fix indentation in raw_reopen_prepare
>     - free rs if raw_reopen_prepare fails
>  v3:
>     - restore suggested-by field missed in v2
> 
>  block/file-win32.c | 90 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 89 insertions(+), 1 deletion(-)

LGTM, asked Helge Konetzka for testing (on the gitlab issue).

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v3] block/file-win32: add reopen handlers

[Helge Konetzka]

Am 17.08.21 um 22:21 schrieb Viktor Prutyanov:
> Make 'qemu-img commit' work on Windows.
>
> Command 'commit' requires reopening backing file in RW mode. So,
> add reopen prepare/commit/abort handlers and change dwShareMode
> for CreateFile call in order to allow further read/write reopening.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/418
> Suggested-by: Hanna Reitz <hreitz@redhat.com>
> Signed-off-by: Viktor Prutyanov <viktor.prutyanov@phystech.edu>
> ---
>   v2:
>      - fix indentation in raw_reopen_prepare
>      - free rs if raw_reopen_prepare fails
>   v3:
>      - restore suggested-by field missed in v2
>
>   block/file-win32.c | 90 +++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 89 insertions(+), 1 deletion(-)

Test was successful on Windows 10 Pro, 21H1, 19043.1165, build & test 
environment MSYS2, MINGW64

Tested-by: Helge Konetzka <hk@zapateado.de>

See my test protocol:

$ git clone https://github.com/patchew-project/qemu.git patchew-qemu
$ cd patchew-qemu
$ git checkout 
tags/patchew/20210817202115.16771-1-viktor.prutyanov@phystech.edu
$ mkdir build-msys2 && cd build-msys2
$ export AR=gcc-ar NM=nm OBJCOPY=objcopy RANLIB=gcc-ranlib STRIP=strip 
WINDRES=windres
$ ../configure \
   --cross-prefix=x86_64-w64-mingw32- \
   --prefix=/mingw64/opt/patchew-qemu --target-list=x86_64-softmmu \
   --bindir=bin --datadir=share/qemu --localedir=share/locale 
--mandir=share/man --docdir=share/doc/qemu
$ make -j2 && make install

$ export PATH="/mingw64/opt/patchew-qemu/bin:$PATH"
$ which qemu-img
/mingw64/opt/patchew-qemu/bin/qemu-img
$ qemu-img create -f qcow2 testbacking.qcow2 20G
Formatting 'testbacking.qcow2', fmt=qcow2 cluster_size=65536 
extended_l2=off compression_type=zlib size=21474836480 
lazy_refcounts=off refcount_bits=16
$ qemu-img create -F qcow2 -b testbacking.qcow2 -f qcow2 test.qcow2
Formatting 'test.qcow2', fmt=qcow2 cluster_size=65536 extended_l2=off 
compression_type=zlib size=21474836480 backing_file=testbacking.qcow2 
backing_fmt=qcow2 lazy_refcounts=off refcount_bits=16
$ ls -l test*.qcow2
-rw-r--r-- 1 User Kein 197120 Aug 18 09:38 test.qcow2
-rw-r--r-- 1 User Kein 197120 Aug 18 09:34 testbacking.qcow2
$ qemu-img commit test.qcow2
Image committed.
$ ls -l test*.qcow2
-rw-r--r-- 1 User Kein 262144 Aug 18 09:39 test.qcow2
-rw-r--r-- 1 User Kein 197120 Aug 18 09:34 testbacking.qcow2
# Install Linux to image ...
$ qemu-system-x86_64 -accel whpx,kernel-irqchip=off -m 1G -hda test.qcow2 \
   -cdrom debian-10.8.0-amd64-netinst.iso
Windows Hypervisor Platform accelerator is operational
# Test Linux image before commit  ...
$ qemu-system-x86_64 -accel whpx,kernel-irqchip=off -m 1G -hda test.qcow2
Windows Hypervisor Platform accelerator is operational
$ ls -l test*.qcow2
-rw-r--r-- 1 User Kein 6641090560 Aug 18 10:57 test.qcow2
-rw-r--r-- 1 User Kein     197120 Aug 18 09:34 testbacking.qcow2
$ qemu-img commit test.qcow2
Image committed.
$ ls -l test*.qcow2
-rw-r--r-- 1 User Kein     262144 Aug 18 11:09 test.qcow2
-rw-r--r-- 1 User Kein 6641090560 Aug 18 11:09 testbacking.qcow2
$ rm test.qcow2
# Test Linux image after commit ...
$ qemu-system-x86_64 -accel whpx,kernel-irqchip=off -m 1G -hda 
testbacking.qcow2

Re: [PATCH v3] block/file-win32: add reopen handlers

[Hanna Reitz]

On 17.08.21 22:21, Viktor Prutyanov wrote:
> Make 'qemu-img commit' work on Windows.
>
> Command 'commit' requires reopening backing file in RW mode. So,
> add reopen prepare/commit/abort handlers and change dwShareMode
> for CreateFile call in order to allow further read/write reopening.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/418
> Suggested-by: Hanna Reitz <hreitz@redhat.com>
> Signed-off-by: Viktor Prutyanov <viktor.prutyanov@phystech.edu>
> ---
>   v2:
>      - fix indentation in raw_reopen_prepare
>      - free rs if raw_reopen_prepare fails
>   v3:
>      - restore suggested-by field missed in v2
>
>   block/file-win32.c | 90 +++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 89 insertions(+), 1 deletion(-)

Overall, looks good to me, thanks!

I just have some questions/suggestions on places where this patch 
deviates from my draft:

> diff --git a/block/file-win32.c b/block/file-win32.c
> index 2642088bd6..9dcbb2b53b 100644
> --- a/block/file-win32.c
> +++ b/block/file-win32.c
> @@ -58,6 +58,10 @@ typedef struct BDRVRawState {
>       QEMUWin32AIOState *aio;
>   } BDRVRawState;
>   
> +typedef struct BDRVRawReopenState {
> +    HANDLE hfile;
> +} BDRVRawReopenState;
> +
>   /*
>    * Read/writes the data to/from a given linear buffer.
>    *
> @@ -392,7 +396,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
>       }
>   
>       s->hfile = CreateFile(filename, access_flags,
> -                          FILE_SHARE_READ, NULL,
> +                          FILE_SHARE_READ | FILE_SHARE_WRITE, NULL,
>                             OPEN_EXISTING, overlapped, NULL);
>       if (s->hfile == INVALID_HANDLE_VALUE) {
>           int err = GetLastError();
> @@ -634,6 +638,86 @@ static int coroutine_fn raw_co_create_opts(BlockDriver *drv,
>       return raw_co_create(&options, errp);
>   }
>   
> +static int raw_reopen_prepare(BDRVReopenState *state,
> +                              BlockReopenQueue *queue, Error **errp)
> +{
> +    BDRVRawState *s = state->bs->opaque;
> +    BDRVRawReopenState *rs;
> +    int access_flags;
> +    DWORD overlapped;
> +    int ret = 0;

Comparing with my original draft 
(https://gitlab.com/hreitz/qemu/-/commit/433ee9a6559dad253e7553692f942dc1824132f0), 
I prevented reopening any node that is not of type FTYPE_FILE (i.e. host 
devices), just to be sure (and because I thought we wouldn’t really need 
other cases).

I don’t strongly lean either way, so perhaps we should indeed just allow 
reopening host devices, but if we do, I think the CreateFile() call in 
hdev_open() should be changed to pass FILE_SHARE_READ | 
FILE_SHARED_WRITE, too (instead of just FILE_SHARE_READ).

> +
> +    rs = g_new0(BDRVRawReopenState, 1);
> +

I had this comment here:

> /*
>  * We do not support changing any options (only flags). By leaving
>  * all options in state->options, we tell the generic reopen code
>  * that we do not support changing any of them, so it will verify
>  * that their values did not change.
>  */

Is there a reason you chose to not include it?  (I think it’s quite nice 
to have this comment, because otherwise it may not be clear why it’s 
“fine” that we don’t look into state->options at all – it’s fine because 
leaving it untouched will make the generic block code verify that 
nothing was changed.)

> +    raw_parse_flags(state->flags, s->aio != NULL, &access_flags, &overlapped);
> +    rs->hfile = CreateFile(state->bs->filename, access_flags,
> +                           FILE_SHARE_READ | FILE_SHARE_WRITE, NULL,
> +                           OPEN_EXISTING, overlapped, NULL);
> +
> +    if (rs->hfile == INVALID_HANDLE_VALUE) {
> +        int err = GetLastError();
> +
> +        error_setg_win32(errp, err, "Could not reopen '%s'",
> +                         state->bs->filename);
> +        if (err == ERROR_ACCESS_DENIED) {
> +            ret = -EACCES;
> +        } else {
> +            ret = -EINVAL;
> +        }
> +        goto fail;
> +    }
> +
> +    if (s->aio) {
> +        ret = win32_aio_attach(s->aio, rs->hfile);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret, "Could not enable AIO");
> +            goto fail;
> +        }
> +    }
> +
> +    state->opaque = rs;
> +
> +    return 0;
> +
> +fail:
> +    g_free(rs);
> +    state->opaque = NULL;
> +
> +    return ret;
> +}
> +
> +static void raw_reopen_commit(BDRVReopenState *state)
> +{
> +    BDRVRawState *s = state->bs->opaque;
> +    BDRVRawReopenState *rs = state->opaque;
> +
> +    if (!rs) {
> +        return;
> +    }

I hope this can’t happen (and I believe it’d be a bug if it did), so I’d 
prefer an

assert(rs != NULL);

instead.

Hanna

> +
> +    CloseHandle(s->hfile);
> +    s->hfile = rs->hfile;
> +
> +    g_free(rs);
> +    state->opaque = NULL;
> +}