Patchwork Improve error handling in do_snapshot_blkdev()

login
register
mail settings
Submitter Jes Sorensen
Date March 3, 2011, 10:44 a.m.
Message ID <1299149091-13979-1-git-send-email-Jes.Sorensen@redhat.com>
Download mbox | patch
Permalink /patch/85253/
State New
Headers show

Comments

Jes Sorensen - March 3, 2011, 10:44 a.m.
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(-)
Stefan Hajnoczi - March 3, 2011, 1:06 p.m.
On Thu, Mar 3, 2011 at 10:44 AM,  <Jes.Sorensen@redhat.com> wrote:
> +    char old_filename[1024];
> +
> +    old_filename[1023] = '\0';
>
>     if (!filename) {
>         qerror_report(QERR_MISSING_PARAMETER, "snapshot_file");
> @@ -591,6 +594,10 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
>         goto out;
>     }
>
> +    strncpy(old_filename, bs->filename, 1024);

strncpy does not NUL-terminate if you reach the maximum length.  The
source buffer is 1024 chars so we should be fine unless there is a bug
somewhere else too, but please move the old_filename[1023] = '\0'
after the strncpy and use sizeof(old_filename) as the maximum instead
of 1024.

Stefan
Jes Sorensen - March 3, 2011, 1:10 p.m.
On 03/03/11 14:06, Stefan Hajnoczi wrote:
> On Thu, Mar 3, 2011 at 10:44 AM,  <Jes.Sorensen@redhat.com> wrote:
>> +    char old_filename[1024];
>> +
>> +    old_filename[1023] = '\0';
>>
>>     if (!filename) {
>>         qerror_report(QERR_MISSING_PARAMETER, "snapshot_file");
>> @@ -591,6 +594,10 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>         goto out;
>>     }
>>
>> +    strncpy(old_filename, bs->filename, 1024);
> 
> strncpy does not NUL-terminate if you reach the maximum length.  The
> source buffer is 1024 chars so we should be fine unless there is a bug
> somewhere else too, but please move the old_filename[1023] = '\0'
> after the strncpy and use sizeof(old_filename) as the maximum instead
> of 1024.

Good point, I was trying to catch it but got it backwards :(

Cheers,
Jes

Patch

diff --git a/blockdev.c b/blockdev.c
index 0690cc8..d43df5e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -574,9 +574,12 @@  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];
+
+    old_filename[1023] = '\0';
 
     if (!filename) {
         qerror_report(QERR_MISSING_PARAMETER, "snapshot_file");
@@ -591,6 +594,10 @@  int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
         goto out;
     }
 
+    strncpy(old_filename, bs->filename, 1024);
+    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) {