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

[Hanna Reitz <hreitz@redhat.com>]

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;
> +}