[v2] Improve error handling in do_snapshot_blkdev()

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(-)

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

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

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