[Qemu-devel] [PATCH v2] Improve error handling in do_snapshot_blkdev()

[Qemu-devel] [PATCH v2] Improve error handling in do_snapshot_blkdev()

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

In case we cannot open the newly created snapshot image, try to fall
back to the original image file and continue running on that, which
should prevent the guest from aborting.

This is a corner case which can happen if the admin by mistake
specifies the snapshot file on a virtual file system which does not
support O_DIRECT. bdrv_create() does not use O_DIRECT, but the
following open in bdrv_open() does and will then fail.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 blockdev.c |   30 ++++++++++++++++++++++++------
 1 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 0690cc8..f52fe8f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -574,9 +574,10 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
     const char *filename = qdict_get_try_str(qdict, "snapshot_file");
     const char *format = qdict_get_try_str(qdict, "format");
     BlockDriverState *bs;
-    BlockDriver *drv, *proto_drv;
+    BlockDriver *drv, *old_drv, *proto_drv;
     int ret = 0;
     int flags;
+    char old_filename[1024];
 
     if (!filename) {
         qerror_report(QERR_MISSING_PARAMETER, "snapshot_file");
@@ -591,6 +592,12 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
         goto out;
     }
 
+    strncpy(old_filename, bs->filename, sizeof(old_filename));
+    old_filename[1023] = '\0';
+
+    old_drv = bs->drv;
+    flags = bs->open_flags;
+
     if (!format) {
         format = "qcow2";
     }
@@ -610,7 +617,7 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
     }
 
     ret = bdrv_img_create(filename, format, bs->filename,
-                          bs->drv->format_name, NULL, -1, bs->open_flags);
+                          bs->drv->format_name, NULL, -1, flags);
     if (ret) {
         goto out;
     }
@@ -618,15 +625,26 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
     qemu_aio_flush();
     bdrv_flush(bs);
 
-    flags = bs->open_flags;
     bdrv_close(bs);
     ret = bdrv_open(bs, filename, flags, drv);
     /*
-     * If reopening the image file we just created fails, we really
-     * are in trouble :(
+     * If reopening the image file we just created fails, fall back
+     * and try to re-open the original image. If that fails too, we
+     * are in serious trouble.
      */
     if (ret != 0) {
-        abort();
+        qerror_report(QERR_OPEN_FILE_FAILED, filename);
+        error_printf("do_snapshot_blkdev(): Unable to open newly created "
+                     "snapshot file: \n");
+        error_printf(" %s. Attempting to revert to original image %s\n",
+                     filename, old_filename);
+        ret = bdrv_open(bs, old_filename, flags, old_drv);
+        if (ret != 0) {
+            error_printf("do_snapshot_blkdev(): Unable to re-open "
+                         "original image - aborting!\n");
+            qerror_report(QERR_OPEN_FILE_FAILED, old_filename);
+            abort();
+        }
     }
 out:
     if (ret) {
-- 
1.7.4

[Qemu-devel] Re: [PATCH v2] Improve error handling in do_snapshot_blkdev()

[Stefan Hajnoczi]

On Thu, Mar 03, 2011 at 02:13:56PM +0100, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> In case we cannot open the newly created snapshot image, try to fall
> back to the original image file and continue running on that, which
> should prevent the guest from aborting.
> 
> This is a corner case which can happen if the admin by mistake
> specifies the snapshot file on a virtual file system which does not
> support O_DIRECT. bdrv_create() does not use O_DIRECT, but the
> following open in bdrv_open() does and will then fail.
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  blockdev.c |   30 ++++++++++++++++++++++++------
>  1 files changed, 24 insertions(+), 6 deletions(-)

Looks good.

Stefan

[Qemu-devel] Re: [PATCH v2] Improve error handling in do_snapshot_blkdev()

[Kevin Wolf]

Am 03.03.2011 14:13, schrieb Jes.Sorensen@redhat.com:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> In case we cannot open the newly created snapshot image, try to fall
> back to the original image file and continue running on that, which
> should prevent the guest from aborting.
> 
> This is a corner case which can happen if the admin by mistake
> specifies the snapshot file on a virtual file system which does not
> support O_DIRECT. bdrv_create() does not use O_DIRECT, but the
> following open in bdrv_open() does and will then fail.
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  blockdev.c |   30 ++++++++++++++++++++++++------
>  1 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 0690cc8..f52fe8f 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -574,9 +574,10 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
>      const char *filename = qdict_get_try_str(qdict, "snapshot_file");
>      const char *format = qdict_get_try_str(qdict, "format");
>      BlockDriverState *bs;
> -    BlockDriver *drv, *proto_drv;
> +    BlockDriver *drv, *old_drv, *proto_drv;
>      int ret = 0;
>      int flags;
> +    char old_filename[1024];
>  
>      if (!filename) {
>          qerror_report(QERR_MISSING_PARAMETER, "snapshot_file");
> @@ -591,6 +592,12 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
>          goto out;
>      }
>  
> +    strncpy(old_filename, bs->filename, sizeof(old_filename));
> +    old_filename[1023] = '\0';

qemu has pstrcpy() from cutils.c for this.

> +
> +    old_drv = bs->drv;
> +    flags = bs->open_flags;
> +
>      if (!format) {
>          format = "qcow2";
>      }
> @@ -610,7 +617,7 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
>      }
>  
>      ret = bdrv_img_create(filename, format, bs->filename,
> -                          bs->drv->format_name, NULL, -1, bs->open_flags);
> +                          bs->drv->format_name, NULL, -1, flags);
>      if (ret) {
>          goto out;
>      }
> @@ -618,15 +625,26 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
>      qemu_aio_flush();
>      bdrv_flush(bs);
>  
> -    flags = bs->open_flags;
>      bdrv_close(bs);
>      ret = bdrv_open(bs, filename, flags, drv);
>      /*
> -     * If reopening the image file we just created fails, we really
> -     * are in trouble :(
> +     * If reopening the image file we just created fails, fall back
> +     * and try to re-open the original image. If that fails too, we
> +     * are in serious trouble.
>       */
>      if (ret != 0) {
> -        abort();
> +        qerror_report(QERR_OPEN_FILE_FAILED, filename);
> +        error_printf("do_snapshot_blkdev(): Unable to open newly created "
> +                     "snapshot file: \n");
> +        error_printf(" %s. Attempting to revert to original image %s\n",

That should probably be a colon in "%s: Attempting..." Also, is the
leading space intentional?

Kevin

[Qemu-devel] Re: [PATCH v2] Improve error handling in do_snapshot_blkdev()

[Jes Sorensen]

On 03/07/11 11:01, Kevin Wolf wrote:
> Am 03.03.2011 14:13, schrieb Jes.Sorensen@redhat.com:
>> @@ -591,6 +592,12 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>          goto out;
>>      }
>>  
>> +    strncpy(old_filename, bs->filename, sizeof(old_filename));
>> +    old_filename[1023] = '\0';
> 
> qemu has pstrcpy() from cutils.c for this.

I'll change it to use pstrcpy().

>> -        abort();
>> +        qerror_report(QERR_OPEN_FILE_FAILED, filename);
>> +        error_printf("do_snapshot_blkdev(): Unable to open newly created "
>> +                     "snapshot file: \n");
>> +        error_printf(" %s. Attempting to revert to original image %s\n",
> 
> That should probably be a colon in "%s: Attempting..." Also, is the
> leading space intentional?

The colon is already there prior to the \n" on the previous printf line.
The space was intentional, but maybe that will just confuse people so I
will remove it. I added a colon after image: in the last line instead.

Look out for v3.


Cheers,
Jes

Re: [Qemu-devel] Re: [PATCH v2] Improve error handling in do_snapshot_blkdev()

[Kevin Wolf]

Am 07.03.2011 16:24, schrieb Jes Sorensen:
> On 03/07/11 11:01, Kevin Wolf wrote:
>> Am 03.03.2011 14:13, schrieb Jes.Sorensen@redhat.com:
>>> @@ -591,6 +592,12 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>>          goto out;
>>>      }
>>>  
>>> +    strncpy(old_filename, bs->filename, sizeof(old_filename));
>>> +    old_filename[1023] = '\0';
>>
>> qemu has pstrcpy() from cutils.c for this.
> 
> I'll change it to use pstrcpy().
> 
>>> -        abort();
>>> +        qerror_report(QERR_OPEN_FILE_FAILED, filename);
>>> +        error_printf("do_snapshot_blkdev(): Unable to open newly created "
>>> +                     "snapshot file: \n");
>>> +        error_printf(" %s. Attempting to revert to original image %s\n",
>>
>> That should probably be a colon in "%s: Attempting..." Also, is the
>> leading space intentional?
> 
> The colon is already there prior to the \n" on the previous printf line.
> The space was intentional, but maybe that will just confuse people so I
> will remove it. I added a colon after image: in the last line instead.

Sorry, I failed to read the context. It's the end of a sentence started
in the line before, so having . instead of : after %s is correct, of course.

Kevin