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

[Anthony Liguori <anthony@codemonkey.ws>]

On 03/07/2011 09:27 AM, 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 |   29 +++++++++++++++++++++++------
>   1 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 0690cc8..d52eef0 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,11 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
>           goto out;
>       }
>
> +    pstrcpy(old_filename, sizeof(old_filename), bs->filename);
> +
> +    old_drv = bs->drv;
> +    flags = bs->open_flags;
> +
>       if (!format) {
>           format = "qcow2";
>       }
> @@ -610,7 +616,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 +624,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);

You can't combine qerror_report with continued action.  qerror_report() 
should be a terminal action.  You also shouldn't combine error_printf() 
with qerror_report().

You should restore the original image file before doing qerror_report() 
and just drop the error_printf()s as it's all redundant information.

Regards,

Anthony Liguori

> +        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) {