From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=33682 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PwdOe-0003p1-TA for qemu-devel@nongnu.org; Mon, 07 Mar 2011 11:35:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PwdOd-0000Fw-Ce for qemu-devel@nongnu.org; Mon, 07 Mar 2011 11:35:00 -0500 Received: from mail-qw0-f45.google.com ([209.85.216.45]:48090) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PwdOd-0000FT-AA for qemu-devel@nongnu.org; Mon, 07 Mar 2011 11:34:59 -0500 Received: by qwj8 with SMTP id 8so3946797qwj.4 for ; Mon, 07 Mar 2011 08:34:58 -0800 (PST) Message-ID: <4D75092F.1020107@codemonkey.ws> Date: Mon, 07 Mar 2011 10:34:55 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH v3] Improve error handling in do_snapshot_blkdev() References: <1299511653-11357-1-git-send-email-Jes.Sorensen@redhat.com> In-Reply-To: <1299511653-11357-1-git-send-email-Jes.Sorensen@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jes.Sorensen@redhat.com Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com On 03/07/2011 09:27 AM, Jes.Sorensen@redhat.com wrote: > From: Jes Sorensen > > 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 > --- > 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) {