From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:33652) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1USp5K-0007WL-2W for qemu-devel@nongnu.org; Thu, 18 Apr 2013 09:41:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1USp5I-0006e9-Mb for qemu-devel@nongnu.org; Thu, 18 Apr 2013 09:41:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55988) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1USp5I-0006dc-FG for qemu-devel@nongnu.org; Thu, 18 Apr 2013 09:41:08 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3IDf7gP020483 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 18 Apr 2013 09:41:07 -0400 Date: Thu, 18 Apr 2013 15:41:04 +0200 From: Kevin Wolf Message-ID: <20130418134104.GI2723@dhcp-200-207.str.redhat.com> References: <6e22db8528d4a801af11b37d42a350eab9633636.1366127809.git.phrdina@redhat.com> <20130418125543.GD2723@dhcp-200-207.str.redhat.com> <516FF2D8.2080003@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <516FF2D8.2080003@redhat.com> Subject: Re: [Qemu-devel] [PATCH 02/11] block: update error reporting for bdrv_snapshot_delete() and related functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Hrdina Cc: lcapitulino@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com Am 18.04.2013 um 15:19 hat Pavel Hrdina geschrieben: > On 18.4.2013 14:55, Kevin Wolf wrote: > >Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben: > >>--- a/block/rbd.c > >>+++ b/block/rbd.c > >>@@ -871,14 +871,19 @@ static int qemu_rbd_snap_create(BlockDriverState *bs, > >> return 0; > >> } > >> > >>-static int qemu_rbd_snap_remove(BlockDriverState *bs, > >>- const char *snapshot_name) > >>+static void qemu_rbd_snap_remove(BlockDriverState *bs, > >>+ const char *snapshot_name, > >>+ Error **errp) > >> { > >> BDRVRBDState *s = bs->opaque; > >> int r; > >> > >> r = rbd_snap_remove(s->image, snapshot_name); > >>- return r; > >>+ if (r < 0) { > >>+ error_setg_errno(errp, -r, "rbd: failed to remove snapshot '%s' on " > >>+ "device '%s'", snapshot_name, > >>+ bdrv_get_device_name(bs)); > > > >Remove the device name. You didn't have it in the qcow2 errors either. > > Or maybe I should also add the device name in the qcow2 errors, > because as Eric write to you these function are used also for > vm-snapshot-delete and devlm and knowing which device failed is > important. I don't think it's the right approach to let the lowest layer add all information that is potentially useful in some contexts. It should only consider what information is essential for its direct caller. You are right that in delvm multiple devices can be affected. This is why delvm should amend the error message with any information that is required for the error message to be helpful in the context of delvm. This is essentially what I suggested here: > >>@@ -2225,18 +2225,17 @@ static int del_existing_snapshots(Monitor *mon, const char *name) > >> { > >> BlockDriverState *bs; > >> QEMUSnapshotInfo sn1, *snapshot = &sn1; > >>- int ret; > >>+ Error *local_err = NULL; > >> > >> bs = NULL; > >> while ((bs = bdrv_next(bs))) { > >> if (bdrv_can_snapshot(bs) && > >> bdrv_snapshot_find(bs, snapshot, name) >= 0) > >> { > >>- ret = bdrv_snapshot_delete(bs, name); > >>- if (ret < 0) { > >>- monitor_printf(mon, > >>- "Error while deleting snapshot on '%s'\n", > >>- bdrv_get_device_name(bs)); > >>+ bdrv_snapshot_delete(bs, name, &local_err); > >>+ if (error_is_set(&local_err)) { > >>+ monitor_printf(mon, "%s\n", error_get_pretty(local_err)); > > > >Here the additional monitor_printf() actually had meaningful additional > >information. Deleting an old snapshot is an implicitly taken action and > >not explicitly requested, so an error message should indicate that it > >happened during the deletion. Maybe something like: > > Function del_existing_snapshots will be anyway dropped later in > patch series so this has no actual value. But I should probably make > something similar to this for HMP command savevm. > > Thanks > > > > >qerror_report(ERROR_CLASS_GENERIC_ERROR, > > "Error while deleting old snapshot on device '%s': %s", > > bdrv_get_device_name(bs), error_get_pretty(local_err)); See how I added the device name and the context of the error ("while deleteing the old snapshot") only in the place where it's actually necessary for the caller to understand the error? By the way, I'm also unsure if the "qcow2:" or "rbd:" prefix is actually helpful. Kevin