From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54659) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1USokH-000709-46 for qemu-devel@nongnu.org; Thu, 18 Apr 2013 09:19:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1USokF-0006lW-7Q for qemu-devel@nongnu.org; Thu, 18 Apr 2013 09:19:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:17592) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1USokE-0006lL-Vr for qemu-devel@nongnu.org; Thu, 18 Apr 2013 09:19:23 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3IDJMue010963 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 18 Apr 2013 09:19:22 -0400 Message-ID: <516FF2D8.2080003@redhat.com> Date: Thu, 18 Apr 2013 15:19:20 +0200 From: Pavel Hrdina MIME-Version: 1.0 References: <6e22db8528d4a801af11b37d42a350eab9633636.1366127809.git.phrdina@redhat.com> <20130418125543.GD2723@dhcp-200-207.str.redhat.com> In-Reply-To: <20130418125543.GD2723@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: Kevin Wolf Cc: lcapitulino@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com On 18.4.2013 14:55, Kevin Wolf wrote: > Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben: >> >> /* >> @@ -567,14 +573,18 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) >> ret = qcow2_update_snapshot_refcount(bs, sn.l1_table_offset, >> sn.l1_size, -1); >> if (ret < 0) { >> - return ret; >> + error_setg_errno(errp, -ret, >> + "qcow2: failed to update snapshot refcount"); > > I'd make it "qcow2: Failed to update refcounts". It's the refcounts of > all clusters referred to by the snapshot's L1 table, not of the snapshot > itself. Thanks for correction. I'm not that much familiar with qcow2 internals. > >> + return; >> } >> qcow2_free_clusters(bs, sn.l1_table_offset, sn.l1_size * sizeof(uint64_t)); >> >> /* must update the copied flag on the current cluster offsets */ >> ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 0); >> if (ret < 0) { >> - return ret; >> + error_setg_errno(errp, -ret, >> + "qcow2: failed to update snapshot refcount"); > > "qcow2: Failed to update cluster flags" Again thanks for correction. > >> + return; >> } >> >> #ifdef DEBUG_ALLOC >> @@ -583,7 +593,6 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) >> qcow2_check_refcounts(bs, &result, 0); >> } >> #endif >> - return 0; >> } >> >> int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab) >> diff --git a/block/qcow2.h b/block/qcow2.h >> index 9421843..dbd332d 100644 >> --- a/block/qcow2.h >> +++ b/block/qcow2.h >> @@ -384,7 +384,9 @@ int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors); >> /* qcow2-snapshot.c functions */ >> int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info); >> int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id); >> -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id); >> +void qcow2_snapshot_delete(BlockDriverState *bs, >> + const char *snapshot_id, >> + Error **errp); >> int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab); >> int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name); >> >> diff --git a/block/rbd.c b/block/rbd.c >> index 141b488..c10edbf 100644 >> --- 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. > >> + } >> } >> >> static int qemu_rbd_snap_rollback(BlockDriverState *bs, >> diff --git a/block/sheepdog.c b/block/sheepdog.c >> index 1c5b532..270fa64 100644 >> --- a/block/sheepdog.c >> +++ b/block/sheepdog.c >> @@ -1908,10 +1908,12 @@ out: >> return ret; >> } >> --- >> diff --git a/savevm.c b/savevm.c >> index 53515cb..6af84fd 100644 >> --- a/savevm.c >> +++ b/savevm.c >> @@ -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)); > >> + error_free(local_err); >> return -1; >> } >> } >> @@ -2450,7 +2449,7 @@ int load_vmstate(const char *name) >> void do_delvm(Monitor *mon, const QDict *qdict) >> { >> BlockDriverState *bs, *bs1; >> - int ret; >> + Error *local_err = NULL; >> const char *name = qdict_get_str(qdict, "name"); >> >> bs = bdrv_snapshots(); >> @@ -2462,15 +2461,10 @@ void do_delvm(Monitor *mon, const QDict *qdict) >> bs1 = NULL; >> while ((bs1 = bdrv_next(bs1))) { >> if (bdrv_can_snapshot(bs1)) { >> - ret = bdrv_snapshot_delete(bs1, name); >> - if (ret < 0) { >> - if (ret == -ENOTSUP) >> - monitor_printf(mon, >> - "Snapshots not supported on device '%s'\n", >> - bdrv_get_device_name(bs1)); >> - else >> - monitor_printf(mon, "Error %d while deleting snapshot on " >> - "'%s'\n", ret, bdrv_get_device_name(bs1)); >> + bdrv_snapshot_delete(bs1, name, &local_err); >> + if (error_is_set(&local_err)) { >> + monitor_printf(mon, "%s\n", error_get_pretty(local_err)); > > Either something like above, indicating the device name on which > bdrv_snapshot_delete() failed, or qerror_report_err(). Like I wrote few lines above, I should add the device name into all errors also in qcow2, rbd and sheepdog. This also applies for bdrv_snapshot_goto/create/list. Pavel > >> + error_free(local_err); >> } >> } >> } > > Kevin >