From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37853) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vd9GI-00072b-EN for qemu-devel@nongnu.org; Sun, 03 Nov 2013 20:47:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vd9G4-0000PI-Ec for qemu-devel@nongnu.org; Sun, 03 Nov 2013 20:47:26 -0500 Received: from e28smtp03.in.ibm.com ([122.248.162.3]:46671) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vd9G3-0000Ot-Gw for qemu-devel@nongnu.org; Sun, 03 Nov 2013 20:47:12 -0500 Received: from /spool/local by e28smtp03.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 4 Nov 2013 07:17:01 +0530 Received: from d28relay05.in.ibm.com (d28relay05.in.ibm.com [9.184.220.62]) by d28dlp01.in.ibm.com (Postfix) with ESMTP id 96937E0055 for ; Mon, 4 Nov 2013 07:18:42 +0530 (IST) Received: from d28av02.in.ibm.com (d28av02.in.ibm.com [9.184.220.64]) by d28relay05.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id rA41ktGr47120590 for ; Mon, 4 Nov 2013 07:16:56 +0530 Received: from d28av02.in.ibm.com (localhost [127.0.0.1]) by d28av02.in.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id rA41kv7J002130 for ; Mon, 4 Nov 2013 07:16:58 +0530 Message-ID: <5276FC94.8030207@linux.vnet.ibm.com> Date: Mon, 04 Nov 2013 09:47:00 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1381787553-12497-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1381787553-12497-2-git-send-email-xiawenc@linux.vnet.ibm.com> <5274F292.9050003@redhat.com> In-Reply-To: <5274F292.9050003@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V4 1/6] snapshot: add parameter *errp in snapshot create List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-devel@nongnu.org Cc: kwolf@redhat.com, pbonzini@redhat.com, stefanha@gmail.com 于 2013/11/2 20:39, Max Reitz 写道: > On 14.10.2013 23:52, Wenchao Xia wrote: >> The return value is only used for error report before this patch, >> so change the function protype to return void. >> >> Signed-off-by: Wenchao Xia >> --- >> block/qcow2-snapshot.c | 27 +++++++++++++++++++++++---- >> block/qcow2.h | 4 +++- >> block/rbd.c | 21 ++++++++++++--------- >> block/sheepdog.c | 29 ++++++++++++++++++++--------- >> block/snapshot.c | 19 +++++++++++++------ >> blockdev.c | 10 ++++------ >> include/block/block_int.h | 5 +++-- >> include/block/snapshot.h | 5 +++-- >> qemu-img.c | 10 ++++++---- >> savevm.c | 12 ++++++++---- >> 10 files changed, 95 insertions(+), 47 deletions(-) >> >> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c >> index 3529c68..b373f9a 100644 >> --- a/block/qcow2-snapshot.c >> +++ b/block/qcow2-snapshot.c >> @@ -347,7 +347,9 @@ static int find_snapshot_by_id_or_name(BlockDriverState *bs, >> } >> >> /* if no id is provided, a new one is constructed */ >> -int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) >> +void qcow2_snapshot_create(BlockDriverState *bs, >> + QEMUSnapshotInfo *sn_info, >> + Error **errp) >> { >> BDRVQcowState *s = bs->opaque; >> QCowSnapshot *new_snapshot_list = NULL; >> @@ -366,7 +368,8 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) >> >> /* Check that the ID is unique */ >> if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) { >> - return -EEXIST; >> + error_setg(errp, "Snapshot with id %s already exist", sn_info->id_str); >> + return; >> } >> >> /* Populate sn with passed data */ >> @@ -383,6 +386,9 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) >> l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t)); >> if (l1_table_offset < 0) { >> ret = l1_table_offset; >> + error_setg(errp, >> + "Failed in allocation of snapshot L1 table: %d (%s)", >> + ret, strerror(-ret)); > > You may want to use error_setg_errno instead (here and in many other > places in this patch). > I forgot we have error_setg_errno(), will use it. >> goto fail; >> } >> >> @@ -397,12 +403,20 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) >> ret = qcow2_pre_write_overlap_check(bs, 0, sn->l1_table_offset, >> s->l1_size * sizeof(uint64_t)); >> if (ret < 0) { >> + error_setg(errp, "Failed in overlap check for snapshot L1 table at %" >> + PRIu64 " with size %" PRIu64 ": %d (%s)", >> + sn->l1_table_offset, s->l1_size * sizeof(uint64_t), >> + ret, strerror(-ret)); > > s->l1_size * sizeof(uint64_t) is of the same type as uint64_t (PRIu64) > only on 64 bit systems. On 32 bit systems, it probably is the same as > uint32_t (because of a different size_t width). I haven't a 32 bit system at hand to test, do you think (int64_t)(s->l1_size * sizeof(uint64_t)) could work for both 32 bit and 64 bit system? > >> goto fail; >> } >> >> ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table, >> s->l1_size * sizeof(uint64_t)); >> if (ret < 0) { >> + error_setg(errp, "Failed in update of snapshot L1 table at %" >> + PRIu64 " with size %" PRIu64 ": %d (%s)", >> + sn->l1_table_offset, s->l1_size * sizeof(uint64_t), >> + ret, strerror(-ret)); > > Same here. > >> goto fail; >> } >> >> @@ -416,6 +430,9 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) >> */ >> ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1); >> if (ret < 0) { >> + error_setg(errp, "Failed in update of refcount for snapshot at %" >> + PRIu64 " with size %d: %d (%s)", >> + s->l1_table_offset, s->l1_size, ret, strerror(-ret)); >> goto fail; >> } >> >> @@ -431,6 +448,8 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) >> >> ret = qcow2_write_snapshots(bs); >> if (ret < 0) { >> + /* Following line will be replaced with more detailed error later */ >> + error_setg(errp, "Failed in write of snapshot"); >> g_free(s->snapshots); >> s->snapshots = old_snapshot_list; >> s->nb_snapshots--; >> @@ -452,14 +471,14 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) >> qcow2_check_refcounts(bs, &result, 0); >> } >> #endif >> - return 0; >> + return; >> >> fail: >> g_free(sn->id_str); >> g_free(sn->name); >> g_free(l1_table); >> >> - return ret; >> + return; >> } >> >> /* copy the snapshot 'snapshot_name' into the current disk image */ >> diff --git a/block/qcow2.h b/block/qcow2.h >> index 922e190..c0a3d01 100644 >> --- a/block/qcow2.h >> +++ b/block/qcow2.h >> @@ -481,7 +481,9 @@ int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors); >> int qcow2_expand_zero_clusters(BlockDriverState *bs); >> >> /* qcow2-snapshot.c functions */ >> -int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info); >> +void qcow2_snapshot_create(BlockDriverState *bs, >> + QEMUSnapshotInfo *sn_info, >> + Error **errp); >> int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id); >> int qcow2_snapshot_delete(BlockDriverState *bs, >> const char *snapshot_id, >> diff --git a/block/rbd.c b/block/rbd.c >> index 4a1ea5b..6181999 100644 >> --- a/block/rbd.c >> +++ b/block/rbd.c >> @@ -860,14 +860,16 @@ static int qemu_rbd_truncate(BlockDriverState *bs, int64_t offset) >> return 0; >> } >> >> -static int qemu_rbd_snap_create(BlockDriverState *bs, >> - QEMUSnapshotInfo *sn_info) >> +static void qemu_rbd_snap_create(BlockDriverState *bs, >> + QEMUSnapshotInfo *sn_info, >> + Error **errp) >> { >> BDRVRBDState *s = bs->opaque; >> int r; >> >> if (sn_info->name[0] == '\0') { >> - return -EINVAL; /* we need a name for rbd snapshots */ >> + error_setg(errp, "rbd snapshot need a valid name"); > > *needs (or "requires") > will change. >> + return; /* we need a name for rbd snapshots */ >> } >> >> /* >> @@ -876,20 +878,21 @@ static int qemu_rbd_snap_create(BlockDriverState *bs, >> */ >> if (sn_info->id_str[0] != '\0' && >> strcmp(sn_info->id_str, sn_info->name) != 0) { >> - return -EINVAL; >> + error_setg(errp, "rbd snapshot id should be empty or equal to name"); >> + return; >> } >> >> if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) { >> - return -ERANGE; >> + error_setg(errp, "rbd snapshot name is too long"); >> + return; >> } >> >> r = rbd_snap_create(s->image, sn_info->name); >> if (r < 0) { >> - error_report("failed to create snap: %s", strerror(-r)); >> - return r; >> + error_setg(errp, >> + "Failed to create snapshot: %d (%s)", >> + r, strerror(-r)); >> } >> - >> - return 0; >> } >> >> static int qemu_rbd_snap_remove(BlockDriverState *bs, >> diff --git a/block/sheepdog.c b/block/sheepdog.c >> index 5f81c93..67aaeb7 100644 >> --- a/block/sheepdog.c >> +++ b/block/sheepdog.c >> @@ -1961,7 +1961,9 @@ static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs) >> return acb->ret; >> } >> >> -static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) >> +static void sd_snapshot_create(BlockDriverState *bs, >> + QEMUSnapshotInfo *sn_info, >> + Error **errp) >> { >> BDRVSheepdogState *s = bs->opaque; >> int ret, fd; >> @@ -1974,10 +1976,10 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) >> s->name, sn_info->vm_state_size, s->is_snapshot); >> >> if (s->is_snapshot) { >> - error_report("You can't create a snapshot of a snapshot VDI, " >> - "%s (%" PRIu32 ").", s->name, s->inode.vdi_id); >> + error_setg(errp, "You can't create a snapshot of a snapshot VDI, " >> + "%s (%" PRIu32 ")", s->name, s->inode.vdi_id); >> >> - return -EINVAL; >> + return; >> } >> >> DPRINTF("%s %s\n", sn_info->name, sn_info->id_str); >> @@ -1995,21 +1997,27 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) >> fd = connect_to_sdog(s); >> if (fd < 0) { >> ret = fd; >> + error_setg(errp, "Failed to connect: %d (%s)", ret, strerror(-ret)); > > You may remove the "ret = fd" line before the error_setg(), if you want > to; but then you'd have to replace the ret in the error_setg() by fd (it > should be an error_setg_errno() again, anyway). Do as you please. > The function is return void now, it seems "ret = fd" can be removed, will do that. >> goto cleanup; >> } >> >> ret = write_object(fd, (char *)&s->inode, vid_to_vdi_oid(s->inode.vdi_id), >> s->inode.nr_copies, datalen, 0, false, s->cache_flags); >> if (ret < 0) { >> - error_report("failed to write snapshot's inode."); >> + error_setg(errp, >> + "Failed to write snapshot's inode: %d (%s)", >> + ret, strerror(-ret)); >> goto cleanup; >> } >> >> ret = do_sd_create(s, s->name, s->inode.vdi_size, s->inode.vdi_id, &new_vid, >> 1); >> if (ret < 0) { >> - error_report("failed to create inode for snapshot. %s", >> - strerror(errno)); >> + error_setg(errp, >> + "Failed to create inode for snapshot: %d (%s), " >> + "errno %d (%s)", >> + ret, strerror(-ret), >> + errno, strerror(errno)); > > This and the following hunk are actually places, where error_setg() > seems better than error_setg_errno() (because it's unclear whether -ret > or errno contains the true error value). > I guess change it as: + error_setg(errp, + "Failed to create inode for snapshot, return is %d", + ret); ? > Max > >> goto cleanup; >> } >> >> @@ -2019,7 +2027,10 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) >> s->inode.nr_copies, datalen, 0, s->cache_flags); >> >> if (ret < 0) { >> - error_report("failed to read new inode info. %s", strerror(errno)); >> + error_setg(errp, "Failed to read new inode info: %d (%s), " >> + "errno %d (%s)", >> + ret, strerror(-ret), >> + errno, strerror(errno)); >> goto cleanup; >> } >> >> @@ -2029,7 +2040,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) >> >> cleanup: >> closesocket(fd); >> - return ret; >> + return; >> } >> >> /* >> diff --git a/block/snapshot.c b/block/snapshot.c >> index a05c0c0..3122e3c 100644 >> --- a/block/snapshot.c >> +++ b/block/snapshot.c >> @@ -138,20 +138,27 @@ int bdrv_can_snapshot(BlockDriverState *bs) >> return 1; >> } >> >> -int bdrv_snapshot_create(BlockDriverState *bs, >> - QEMUSnapshotInfo *sn_info) >> +void bdrv_snapshot_create(BlockDriverState *bs, >> + QEMUSnapshotInfo *sn_info, >> + Error **errp) >> { >> BlockDriver *drv = bs->drv; >> if (!drv) { >> - return -ENOMEDIUM; >> + error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs)); >> + return; >> } >> if (drv->bdrv_snapshot_create) { >> - return drv->bdrv_snapshot_create(bs, sn_info); >> + drv->bdrv_snapshot_create(bs, sn_info, errp); >> + return; >> } >> if (bs->file) { >> - return bdrv_snapshot_create(bs->file, sn_info); >> + bdrv_snapshot_create(bs->file, sn_info, errp); >> + return; >> } >> - return -ENOTSUP; >> + >> + error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED, >> + drv->format_name, bdrv_get_device_name(bs), >> + "internal snapshot creation"); >> } >> >> int bdrv_snapshot_goto(BlockDriverState *bs, >> diff --git a/blockdev.c b/blockdev.c >> index 4f76e28..c4e7124 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -1077,7 +1077,7 @@ static void internal_snapshot_prepare(BlkTransactionState *common, >> qemu_timeval tv; >> BlockdevSnapshotInternal *internal; >> InternalSnapshotState *state; >> - int ret1; >> + Error *local_err = NULL; >> >> g_assert(common->action->kind == >> TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC); >> @@ -1135,11 +1135,9 @@ static void internal_snapshot_prepare(BlkTransactionState *common, >> sn->date_nsec = tv.tv_usec * 1000; >> sn->vm_clock_nsec = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); >> >> - ret1 = bdrv_snapshot_create(bs, sn); >> - if (ret1 < 0) { >> - error_setg_errno(errp, -ret1, >> - "Failed to create snapshot '%s' on device '%s'", >> - name, device); >> + bdrv_snapshot_create(bs, sn, &local_err); >> + if (error_is_set(&local_err)) { >> + error_propagate(errp, local_err); >> return; >> } >> >> diff --git a/include/block/block_int.h b/include/block/block_int.h >> index a48731d..553d843 100644 >> --- a/include/block/block_int.h >> +++ b/include/block/block_int.h >> @@ -161,8 +161,9 @@ struct BlockDriver { >> int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num, >> const uint8_t *buf, int nb_sectors); >> >> - int (*bdrv_snapshot_create)(BlockDriverState *bs, >> - QEMUSnapshotInfo *sn_info); >> + void (*bdrv_snapshot_create)(BlockDriverState *bs, >> + QEMUSnapshotInfo *sn_info, >> + Error **errp); >> int (*bdrv_snapshot_goto)(BlockDriverState *bs, >> const char *snapshot_id); >> int (*bdrv_snapshot_delete)(BlockDriverState *bs, >> diff --git a/include/block/snapshot.h b/include/block/snapshot.h >> index 012bf22..a998316 100644 >> --- a/include/block/snapshot.h >> +++ b/include/block/snapshot.h >> @@ -47,8 +47,9 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs, >> QEMUSnapshotInfo *sn_info, >> Error **errp); >> int bdrv_can_snapshot(BlockDriverState *bs); >> -int bdrv_snapshot_create(BlockDriverState *bs, >> - QEMUSnapshotInfo *sn_info); >> +void bdrv_snapshot_create(BlockDriverState *bs, >> + QEMUSnapshotInfo *sn_info, >> + Error **errp); >> int bdrv_snapshot_goto(BlockDriverState *bs, >> const char *snapshot_id); >> int bdrv_snapshot_delete(BlockDriverState *bs, >> diff --git a/qemu-img.c b/qemu-img.c >> index 926f0a0..aa8b46f 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -2080,10 +2080,12 @@ static int img_snapshot(int argc, char **argv) >> sn.date_sec = tv.tv_sec; >> sn.date_nsec = tv.tv_usec * 1000; >> >> - ret = bdrv_snapshot_create(bs, &sn); >> - if (ret) { >> - error_report("Could not create snapshot '%s': %d (%s)", >> - snapshot_name, ret, strerror(-ret)); >> + bdrv_snapshot_create(bs, &sn, &err); >> + if (error_is_set(&err)) { >> + error_report("Could not create snapshot '%s': %s", >> + snapshot_name, error_get_pretty(err)); >> + error_free(err); >> + ret = 1; >> } >> break; >> >> diff --git a/savevm.c b/savevm.c >> index 2f631d4..617d6eb 100644 >> --- a/savevm.c >> +++ b/savevm.c >> @@ -2366,6 +2366,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) >> qemu_timeval tv; >> struct tm tm; >> const char *name = qdict_get_try_str(qdict, "name"); >> + Error *err = NULL; >> >> /* Verify if there is a device that doesn't support snapshots and is writable */ >> bs = NULL; >> @@ -2439,10 +2440,13 @@ void do_savevm(Monitor *mon, const QDict *qdict) >> if (bdrv_can_snapshot(bs1)) { >> /* Write VM state size only to the image that contains the state */ >> sn->vm_state_size = (bs == bs1 ? vm_state_size : 0); >> - ret = bdrv_snapshot_create(bs1, sn); >> - if (ret < 0) { >> - monitor_printf(mon, "Error while creating snapshot on '%s'\n", >> - bdrv_get_device_name(bs1)); >> + bdrv_snapshot_create(bs1, sn, &err); >> + if (error_is_set(&err)) { >> + monitor_printf(mon, >> + "Error while creating snapshot on '%s': %s\n", >> + bdrv_get_device_name(bs1), >> + error_get_pretty(err)); >> + error_free(err); >> } >> } >> } >