[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3] block/file-win32: add reopen handlers
From: |
Hanna Reitz |
Subject: |
Re: [PATCH v3] block/file-win32: add reopen handlers |
Date: |
Thu, 19 Aug 2021 14:51:06 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 |
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;
+}