| Message ID | 20210817202115.16771-1-viktor.prutyanov@phystech.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] block/file-win32: add reopen handlers | expand |
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>
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
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; > +}
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,
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(-)