Patchwork [v4,15/19] block: raw-win32 driver reopen support

login
register
mail settings
Submitter Jeff Cody
Date Sept. 20, 2012, 7:13 p.m.
Message ID <71de241ac2e46041abdddd36683b2a1f82fb1e2c.1348157913.git.jcody@redhat.com>
Download mbox | patch
Permalink /patch/185465/
State New
Headers show

Comments

Jeff Cody - Sept. 20, 2012, 7:13 p.m.
This is the support for reopen, for win32.  Note that there is a key
difference in the win32 reopen, namely that it is not guaranteed safe
like all the other drivers.  Attempts are made to reopen the file, or
open the file in a new handle prior to closing the old handle.  However,
this is not always feasible, and there are times when we must close the
existing file and then open the new file, breaking the transactional nature
of the reopen.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/raw-win32.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 105 insertions(+)
Kevin Wolf - Sept. 21, 2012, 8:33 a.m.
Am 20.09.2012 21:13, schrieb Jeff Cody:
> This is the support for reopen, for win32.  Note that there is a key
> difference in the win32 reopen, namely that it is not guaranteed safe
> like all the other drivers.  Attempts are made to reopen the file, or
> open the file in a new handle prior to closing the old handle.  However,
> this is not always feasible, and there are times when we must close the
> existing file and then open the new file, breaking the transactional nature
> of the reopen.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/raw-win32.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 105 insertions(+)

I can't really review win32 code, but two comments anyway:

> diff --git a/block/raw-win32.c b/block/raw-win32.c
> index 78c8306..8a698d3 100644
> --- a/block/raw-win32.c
> +++ b/block/raw-win32.c
> @@ -31,6 +31,7 @@
>  #define FTYPE_FILE 0
>  #define FTYPE_CD     1
>  #define FTYPE_HARDDISK 2
> +#define WINDOWS_VISTA 6
>  
>  typedef struct BDRVRawState {
>      HANDLE hfile;
> @@ -38,6 +39,10 @@ typedef struct BDRVRawState {
>      char drive_path[16]; /* format: "d:\" */
>  } BDRVRawState;
>  
> +typedef struct BDRVRawReopenState {
> +    HANDLE hfile;
> +} BDRVRawReopenState;
> +
>  int qemu_ftruncate64(int fd, int64_t length)
>  {
>      LARGE_INTEGER li;
> @@ -117,6 +122,103 @@ static int raw_open(BlockDriverState *bs, const char *filename, int flags)
>      return 0;
>  }
>  
> +static int raw_reopen_prepare(BDRVReopenState *state,
> +                              BlockReopenQueue *queue, Error **errp)
> +{
> +    BDRVRawState *s;
> +    BDRVRawReopenState *raw_s;
> +    int ret = 0;
> +    int access_flags;
> +    DWORD overlapped;
> +    OSVERSIONINFO osvi;
> +
> +    assert(state != NULL);
> +    assert(state->bs != NULL);
> +
> +    s = state->bs->opaque;
> +
> +    state->opaque = g_malloc0(sizeof(BDRVRawReopenState));
> +    raw_s = state->opaque;
> +
> +    raw_parse_flags(state->flags, &access_flags, &overlapped);
> +
> +    ZeroMemory(&osvi, sizeof(OSVERSIONINFO));
> +    osvi.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
> +
> +    GetVersionEx(&osvi);
> +    raw_s->hfile = INVALID_HANDLE_VALUE;
> +
> +    if (osvi.dwMajorVersion >= WINDOWS_VISTA) {
> +        raw_s->hfile = ReOpenFile(s->hfile, access_flags, FILE_SHARE_READ,
> +                                  overlapped);
> +    }
> +
> +    /* could not reopen the file handle, so fall back to opening
> +     * new file (CreateFile) */
> +    if (raw_s->hfile == INVALID_HANDLE_VALUE) {
> +        raw_s->hfile = CreateFile(state->bs->filename, access_flags,
> +                                  FILE_SHARE_READ, NULL, OPEN_EXISTING,
> +                                  overlapped, NULL);
> +        if (raw_s->hfile == INVALID_HANDLE_VALUE) {
> +            /* this could happen because the access_flags requested are
> +             * incompatible with the existing share mode of s->hfile,
> +             * so our only option now is to close s->hfile, and try again.
> +             * This could end badly */
> +            CloseHandle(s->hfile);

How common is this case?

We do have another option, namely not reopen at all and return an error.
Of course, this only makes sense if it doesn't mean that we almost never
succeed.

> +            s->hfile = INVALID_HANDLE_VALUE;
> +            raw_s->hfile = CreateFile(state->bs->filename, access_flags,
> +                                      FILE_SHARE_READ, NULL, OPEN_EXISTING,
> +                                      overlapped, NULL);
> +            if (raw_s->hfile == INVALID_HANDLE_VALUE) {
> +                /* Unrecoverable error */
> +                error_set(errp, ERROR_CLASS_GENERIC_ERROR,
> +                          "Fatal error reopening %s file; file closed and cannot be opened\n",

This line is longer than 80 characters.

Kevin
Paolo Bonzini - Sept. 21, 2012, 8:43 a.m.
Il 21/09/2012 10:33, Kevin Wolf ha scritto:
>> > +    /* could not reopen the file handle, so fall back to opening
>> > +     * new file (CreateFile) */
>> > +    if (raw_s->hfile == INVALID_HANDLE_VALUE) {
>> > +        raw_s->hfile = CreateFile(state->bs->filename, access_flags,
>> > +                                  FILE_SHARE_READ, NULL, OPEN_EXISTING,
>> > +                                  overlapped, NULL);
>> > +        if (raw_s->hfile == INVALID_HANDLE_VALUE) {
>> > +            /* this could happen because the access_flags requested are
>> > +             * incompatible with the existing share mode of s->hfile,
>> > +             * so our only option now is to close s->hfile, and try again.
>> > +             * This could end badly */
>> > +            CloseHandle(s->hfile);
> How common is this case?
> 
> We do have another option, namely not reopen at all and return an error.
> Of course, this only makes sense if it doesn't mean that we almost never
> succeed.

Probably pretty common since we specify FILE_SHARE_READ for the sharing
mode, meaning that "subsequent open operations on a file or device are
only able to request read access".

I would change it to FILE_SHARE_READ|FILE_SHARE_WRITE and remove this code.

Paolo
Jeff Cody - Sept. 21, 2012, 12:17 p.m.
On 09/21/2012 04:43 AM, Paolo Bonzini wrote:
> Il 21/09/2012 10:33, Kevin Wolf ha scritto:
>>>> +    /* could not reopen the file handle, so fall back to opening
>>>> +     * new file (CreateFile) */
>>>> +    if (raw_s->hfile == INVALID_HANDLE_VALUE) {
>>>> +        raw_s->hfile = CreateFile(state->bs->filename, access_flags,
>>>> +                                  FILE_SHARE_READ, NULL, OPEN_EXISTING,
>>>> +                                  overlapped, NULL);
>>>> +        if (raw_s->hfile == INVALID_HANDLE_VALUE) {
>>>> +            /* this could happen because the access_flags requested are
>>>> +             * incompatible with the existing share mode of s->hfile,
>>>> +             * so our only option now is to close s->hfile, and try again.
>>>> +             * This could end badly */
>>>> +            CloseHandle(s->hfile);
>> How common is this case?
>>
>> We do have another option, namely not reopen at all and return an error.
>> Of course, this only makes sense if it doesn't mean that we almost never
>> succeed.
> 
> Probably pretty common since we specify FILE_SHARE_READ for the sharing
> mode, meaning that "subsequent open operations on a file or device are
> only able to request read access".
> 

Yes, I think this is by far the most common case.


> I would change it to FILE_SHARE_READ|FILE_SHARE_WRITE and remove this code.
> 
> Paolo
> 

I contemplated doing that, but I wasn't sure if there was any particular
reason it was originally done with FILE_SHARE_READ only in the first
place (security, etc..). I was hesitant to override that behaviour as
the new default under w32.  Do we know if this is acceptable / safe?
Paolo Bonzini - Sept. 21, 2012, 12:31 p.m.
Il 21/09/2012 14:17, Jeff Cody ha scritto:
> On 09/21/2012 04:43 AM, Paolo Bonzini wrote:
>> Il 21/09/2012 10:33, Kevin Wolf ha scritto:
>>>>> +    /* could not reopen the file handle, so fall back to opening
>>>>> +     * new file (CreateFile) */
>>>>> +    if (raw_s->hfile == INVALID_HANDLE_VALUE) {
>>>>> +        raw_s->hfile = CreateFile(state->bs->filename, access_flags,
>>>>> +                                  FILE_SHARE_READ, NULL, OPEN_EXISTING,
>>>>> +                                  overlapped, NULL);
>>>>> +        if (raw_s->hfile == INVALID_HANDLE_VALUE) {
>>>>> +            /* this could happen because the access_flags requested are
>>>>> +             * incompatible with the existing share mode of s->hfile,
>>>>> +             * so our only option now is to close s->hfile, and try again.
>>>>> +             * This could end badly */
>>>>> +            CloseHandle(s->hfile);
>>> How common is this case?
>>>
>>> We do have another option, namely not reopen at all and return an error.
>>> Of course, this only makes sense if it doesn't mean that we almost never
>>> succeed.
>>
>> Probably pretty common since we specify FILE_SHARE_READ for the sharing
>> mode, meaning that "subsequent open operations on a file or device are
>> only able to request read access".
> 
> Yes, I think this is by far the most common case.

Actually ReOpenFile probably only takes into account _other_ sharing
modes, not the one for hFile, so it may even be unnecessary.

But...

>> I would change it to FILE_SHARE_READ|FILE_SHARE_WRITE and remove this code.
> 
> I contemplated doing that, but I wasn't sure if there was any particular
> reason it was originally done with FILE_SHARE_READ only in the first
> place (security, etc..). I was hesitant to override that behaviour as
> the new default under w32.  Do we know if this is acceptable / safe?

... let's just make things work the same as in Unix.

Paolo

Patch

diff --git a/block/raw-win32.c b/block/raw-win32.c
index 78c8306..8a698d3 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -31,6 +31,7 @@ 
 #define FTYPE_FILE 0
 #define FTYPE_CD     1
 #define FTYPE_HARDDISK 2
+#define WINDOWS_VISTA 6
 
 typedef struct BDRVRawState {
     HANDLE hfile;
@@ -38,6 +39,10 @@  typedef struct BDRVRawState {
     char drive_path[16]; /* format: "d:\" */
 } BDRVRawState;
 
+typedef struct BDRVRawReopenState {
+    HANDLE hfile;
+} BDRVRawReopenState;
+
 int qemu_ftruncate64(int fd, int64_t length)
 {
     LARGE_INTEGER li;
@@ -117,6 +122,103 @@  static int raw_open(BlockDriverState *bs, const char *filename, int flags)
     return 0;
 }
 
+static int raw_reopen_prepare(BDRVReopenState *state,
+                              BlockReopenQueue *queue, Error **errp)
+{
+    BDRVRawState *s;
+    BDRVRawReopenState *raw_s;
+    int ret = 0;
+    int access_flags;
+    DWORD overlapped;
+    OSVERSIONINFO osvi;
+
+    assert(state != NULL);
+    assert(state->bs != NULL);
+
+    s = state->bs->opaque;
+
+    state->opaque = g_malloc0(sizeof(BDRVRawReopenState));
+    raw_s = state->opaque;
+
+    raw_parse_flags(state->flags, &access_flags, &overlapped);
+
+    ZeroMemory(&osvi, sizeof(OSVERSIONINFO));
+    osvi.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
+
+    GetVersionEx(&osvi);
+    raw_s->hfile = INVALID_HANDLE_VALUE;
+
+    if (osvi.dwMajorVersion >= WINDOWS_VISTA) {
+        raw_s->hfile = ReOpenFile(s->hfile, access_flags, FILE_SHARE_READ,
+                                  overlapped);
+    }
+
+    /* could not reopen the file handle, so fall back to opening
+     * new file (CreateFile) */
+    if (raw_s->hfile == INVALID_HANDLE_VALUE) {
+        raw_s->hfile = CreateFile(state->bs->filename, access_flags,
+                                  FILE_SHARE_READ, NULL, OPEN_EXISTING,
+                                  overlapped, NULL);
+        if (raw_s->hfile == INVALID_HANDLE_VALUE) {
+            /* this could happen because the access_flags requested are
+             * incompatible with the existing share mode of s->hfile,
+             * so our only option now is to close s->hfile, and try again.
+             * This could end badly */
+            CloseHandle(s->hfile);
+            s->hfile = INVALID_HANDLE_VALUE;
+            raw_s->hfile = CreateFile(state->bs->filename, access_flags,
+                                      FILE_SHARE_READ, NULL, OPEN_EXISTING,
+                                      overlapped, NULL);
+            if (raw_s->hfile == INVALID_HANDLE_VALUE) {
+                /* Unrecoverable error */
+                error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+                          "Fatal error reopening %s file; file closed and cannot be opened\n",
+                          state->bs->filename);
+                ret = -1;
+            } else{
+                /* since we had to close the original, go ahead and
+                 * re-assign here */
+                s->hfile = raw_s->hfile;
+                raw_s->hfile = INVALID_HANDLE_VALUE;
+            }
+        }
+    }
+
+    return ret;
+}
+
+
+static void raw_reopen_commit(BDRVReopenState *state)
+{
+    BDRVRawReopenState *raw_s = state->opaque;
+    BDRVRawState *s = state->bs->opaque;
+
+    if (raw_s->hfile != INVALID_HANDLE_VALUE) {
+        CloseHandle(s->hfile);
+        s->hfile = raw_s->hfile;
+    }
+
+    g_free(state->opaque);
+    state->opaque = NULL;
+}
+
+
+static void raw_reopen_abort(BDRVReopenState *state)
+{
+    BDRVRawReopenState *raw_s = state->opaque;
+
+     /* nothing to do if NULL, we didn't get far enough */
+    if (raw_s == NULL) {
+        return;
+    }
+
+    if (raw_s->hfile != INVALID_HANDLE_VALUE) {
+        CloseHandle(raw_s->hfile);
+    }
+    g_free(state->opaque);
+    state->opaque = NULL;
+}
+
 static int raw_read(BlockDriverState *bs, int64_t sector_num,
                     uint8_t *buf, int nb_sectors)
 {
@@ -287,6 +389,9 @@  static BlockDriver bdrv_file = {
     .protocol_name	= "file",
     .instance_size	= sizeof(BDRVRawState),
     .bdrv_file_open	= raw_open,
+    .bdrv_reopen_prepare = raw_reopen_prepare,
+    .bdrv_reopen_commit = raw_reopen_commit,
+    .bdrv_reopen_abort = raw_reopen_abort,
     .bdrv_close		= raw_close,
     .bdrv_create	= raw_create,