From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56077) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T9Y4D-0001uM-Em for qemu-devel@nongnu.org; Thu, 06 Sep 2012 05:08:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T9Y46-0003Zn-Kr for qemu-devel@nongnu.org; Thu, 06 Sep 2012 05:08:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48896) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T9Y46-0003Zg-9H for qemu-devel@nongnu.org; Thu, 06 Sep 2012 05:07:58 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q8697vQH017875 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 6 Sep 2012 05:07:57 -0400 Message-ID: <504867EA.5020504@redhat.com> Date: Thu, 06 Sep 2012 11:07:54 +0200 From: Pavel Hrdina MIME-Version: 1.0 References: <3222678753b33b3a95096767745a5cf313c7ca1b.1345016001.git.phrdina@redhat.com> <20120830114701.7f5c1413@doriath.home> In-Reply-To: <20120830114701.7f5c1413@doriath.home> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 02/18] block: add error parameter to bdrv_snapshot_create() and related functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: Kevin Wolf , qemu-devel@nongnu.org, Markus Armbruster On 08/30/2012 04:47 PM, Luiz Capitulino wrote: > On Wed, 15 Aug 2012 09:41:43 +0200 > Pavel Hrdina wrote: > >> Signed-off-by: Pavel Hrdina >> --- >> block.c | 25 +++++++++++++++++-------- >> block.h | 3 ++- >> block/qcow2-snapshot.c | 9 ++++++++- >> block/qcow2.h | 4 +++- >> block/rbd.c | 20 ++++++++++++++------ >> block/sheepdog.c | 17 +++++++++-------- >> block_int.h | 3 ++- >> qemu-img.c | 2 +- >> savevm.c | 2 +- >> 9 files changed, 57 insertions(+), 28 deletions(-) >> >> diff --git a/block.c b/block.c >> index 016858b..8bc49b7 100644 >> --- a/block.c >> +++ b/block.c >> @@ -2661,16 +2661,25 @@ BlockDriverState *bdrv_snapshots(void) >> } >> >> int bdrv_snapshot_create(BlockDriverState *bs, >> - QEMUSnapshotInfo *sn_info) >> + QEMUSnapshotInfo *sn_info, >> + Error **errp) >> { >> BlockDriver *drv = bs->drv; >> - if (!drv) >> - return -ENOMEDIUM; >> - if (drv->bdrv_snapshot_create) >> - return drv->bdrv_snapshot_create(bs, sn_info); >> - if (bs->file) >> - return bdrv_snapshot_create(bs->file, sn_info); >> - return -ENOTSUP; >> + int ret; >> + >> + if (!drv) { >> + error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs)); > We should only use QERR_ macros for the errors listed in the ErrorClass enum > (except GenericError), all other errors should generally use error_setg(), like > this: > > error_setg(errp, "device '%s' has no medium); I agree that we should use error_setg() for all GenericError, but I think that would be better have macros for errors which are used many-times . For example: #define QERR_ERRMSG_DEVICE_HAS_NO_MEDIUM "Device '%s' has no medium" and use error_setg(errp, QERR_ERRMSG_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs)); I think that the consistency of error messages is important and an error message should be the same for every error occurrence. > >> + ret = -ENOMEDIUM; > And, usually, we should get rid of errno propagation. There are two cases here: > > 1. errno is propagated up so that upper layers can print a decent error > message to the user. > > In this case, it's safe to eliminate errno. error_setg() will store a > decent message already and the Error object can be propagated up. > > 2. errno is propagated up so that upper layers can distinguish among > error causes and take different actions accordingly. > > Doesn't seem to be the case of bdrv_snapshot_create() (ie. errno is only > used to communicate the error to the user). However, I'm pretty sure that > such usage exists in qemu and the error API will break it, as most of our > errors are generic. > > I see two solutions to this problem: > > A. Add specific errors to ErrorClass. I don't like this very much, > as it's possible that such errors are going to be useful only internally. > > B. Add two new functions: > > void error_sete(Error **err, ErrorClass err_class, int errno, const char *fmt, ...); > int error_get_errno(const Error **err); > > So that we can maintain errno when it's used to communicate > error cause among functions. > >> + } else if (drv->bdrv_snapshot_create) { >> + ret = drv->bdrv_snapshot_create(bs, sn_info, errp); >> + } else if (bs->file) { >> + ret = bdrv_snapshot_create(bs->file, sn_info, errp); >> + } else { >> + error_set(errp, QERR_NOT_SUPPORTED); >> + ret = -ENOTSUP; >> + } >> + >> + return ret; >> } >> >> int bdrv_snapshot_goto(BlockDriverState *bs, >> diff --git a/block.h b/block.h >> index 2e2be11..92e782b 100644 >> --- a/block.h >> +++ b/block.h >> @@ -296,7 +296,8 @@ int bdrv_can_snapshot(BlockDriverState *bs); >> int bdrv_is_snapshot(BlockDriverState *bs); >> BlockDriverState *bdrv_snapshots(void); >> int bdrv_snapshot_create(BlockDriverState *bs, >> - QEMUSnapshotInfo *sn_info); >> + QEMUSnapshotInfo *sn_info, >> + Error **errp); >> int bdrv_snapshot_goto(BlockDriverState *bs, >> const char *snapshot_id); >> int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id); >> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c >> index 4e7c93b..cf86dae 100644 >> --- a/block/qcow2-snapshot.c >> +++ b/block/qcow2-snapshot.c >> @@ -25,6 +25,7 @@ >> #include "qemu-common.h" >> #include "block_int.h" >> #include "block/qcow2.h" >> +#include "qerror.h" >> >> typedef struct QEMU_PACKED QCowSnapshotHeader { >> /* header is 8 byte aligned */ >> @@ -312,7 +313,9 @@ static int find_snapshot_by_id_or_name(BlockDriverState *bs, const char *name) >> } >> >> /* if no id is provided, a new one is constructed */ >> -int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) >> +int qcow2_snapshot_create(BlockDriverState *bs, >> + QEMUSnapshotInfo *sn_info, >> + Error **errp) >> { >> BDRVQcowState *s = bs->opaque; >> QCowSnapshot *new_snapshot_list = NULL; >> @@ -331,6 +334,8 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) >> >> /* Check that the ID is unique */ >> if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) { >> + error_set(errp, QERR_INVALID_PARAMETER_VALUE, >> + "name", "non-existing id identifier"); >> return -EEXIST; >> } >> >> @@ -415,6 +420,8 @@ fail: >> g_free(sn->name); >> g_free(l1_table); >> >> + error_set(errp, QERR_GENERIC_ERROR, ret); >> + >> return ret; >> } >> >> diff --git a/block/qcow2.h b/block/qcow2.h >> index b4eb654..854bd12 100644 >> --- a/block/qcow2.h >> +++ b/block/qcow2.h >> @@ -308,7 +308,9 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset, >> 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_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); >> int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab); >> diff --git a/block/rbd.c b/block/rbd.c >> index 5a0f79f..7bc42f0 100644 >> --- a/block/rbd.c >> +++ b/block/rbd.c >> @@ -16,6 +16,7 @@ >> #include "qemu-common.h" >> #include "qemu-error.h" >> #include "block_int.h" >> +#include "qerror.h" >> >> #include >> >> @@ -817,12 +818,15 @@ static int qemu_rbd_truncate(BlockDriverState *bs, int64_t offset) >> } >> >> static int qemu_rbd_snap_create(BlockDriverState *bs, >> - QEMUSnapshotInfo *sn_info) >> + QEMUSnapshotInfo *sn_info, >> + Error **errp) >> { >> BDRVRBDState *s = bs->opaque; >> - int r; >> + int ret; >> >> if (sn_info->name[0] == '\0') { >> + error_set(errp, QERR_INVALID_PARAMETER_VALUE, >> + "name", "some tag identifier"); >> return -EINVAL; /* we need a name for rbd snapshots */ >> } >> >> @@ -832,17 +836,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) { >> + error_set(errp, QERR_INVALID_PARAMETER_VALUE, >> + "name", "id and tag to equal"); >> return -EINVAL; >> } >> >> if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) { >> + error_set(errp, QERR_INVALID_PARAMETER_VALUE, >> + "name", "shorter than 127 chars"); >> return -ERANGE; >> } >> >> - r = rbd_snap_create(s->image, sn_info->name); >> - if (r < 0) { >> - error_report("failed to create snap: %s", strerror(-r)); >> - return r; >> + ret = rbd_snap_create(s->image, sn_info->name); >> + if (ret < 0) { >> + error_set(errp, QERR_GENERIC_ERROR, ret); >> + return ret; > You should always be careful when dropping error_report(), most of the time you > also have to change callers. In this case, bdrv_snapshot_create() is called > by qemu-img, which should be changed to print the error from the Error object. > >> } >> >> return 0; >> diff --git a/block/sheepdog.c b/block/sheepdog.c >> index a04ad99..fc51e04 100644 >> --- a/block/sheepdog.c >> +++ b/block/sheepdog.c >> @@ -17,6 +17,7 @@ >> #include "qemu_socket.h" >> #include "block_int.h" >> #include "bitops.h" >> +#include "qerror.h" >> >> #define SD_PROTO_VER 0x01 >> >> @@ -1730,7 +1731,9 @@ static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs) >> return 0; >> } >> >> -static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) >> +static int sd_snapshot_create(BlockDriverState *bs, >> + QEMUSnapshotInfo *sn_info, >> + Error **errp) >> { >> BDRVSheepdogState *s = bs->opaque; >> int ret, fd; >> @@ -1743,9 +1746,8 @@ 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_set(errp, QERR_INVALID_PARAMETER_VALUE, >> + "name", " not VDI snapshot"); >> return -EINVAL; >> } >> >> @@ -1767,15 +1769,12 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) >> ret = write_object(fd, (char *)&s->inode, vid_to_vdi_oid(s->inode.vdi_id), >> s->inode.nr_copies, datalen, 0, 0, s->cache_enabled); >> if (ret < 0) { >> - error_report("failed to write snapshot's inode."); >> goto cleanup; >> } >> >> ret = do_sd_create(s->name, s->inode.vdi_size, s->inode.vdi_id, &new_vid, 1, >> s->addr, s->port); >> if (ret < 0) { >> - error_report("failed to create inode for snapshot. %s", >> - strerror(errno)); >> goto cleanup; >> } >> >> @@ -1785,7 +1784,6 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) >> s->inode.nr_copies, datalen, 0, s->cache_enabled); >> >> if (ret < 0) { >> - error_report("failed to read new inode info. %s", strerror(errno)); >> goto cleanup; >> } >> >> @@ -1795,6 +1793,9 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) >> >> cleanup: >> closesocket(fd); >> + if (ret < 0) { >> + error_set(errp, QERR_GENERIC_ERROR, ret); >> + } >> return ret; >> } >> >> diff --git a/block_int.h b/block_int.h >> index 4452f6f..b76c943 100644 >> --- a/block_int.h >> +++ b/block_int.h >> @@ -206,7 +206,8 @@ struct BlockDriver { >> const uint8_t *buf, int nb_sectors); >> >> int (*bdrv_snapshot_create)(BlockDriverState *bs, >> - QEMUSnapshotInfo *sn_info); >> + QEMUSnapshotInfo *sn_info, >> + Error **errp); >> int (*bdrv_snapshot_goto)(BlockDriverState *bs, >> const char *snapshot_id); >> int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id); >> diff --git a/qemu-img.c b/qemu-img.c >> index b41e670..c798d66 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -1267,7 +1267,7 @@ 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); >> + ret = bdrv_snapshot_create(bs, &sn, NULL); >> if (ret) { >> error_report("Could not create snapshot '%s': %d (%s)", >> snapshot_name, ret, strerror(-ret)); >> diff --git a/savevm.c b/savevm.c >> index 0ea10c9..1b67af6 100644 >> --- a/savevm.c >> +++ b/savevm.c >> @@ -2165,7 +2165,7 @@ 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); >> + ret = bdrv_snapshot_create(bs1, sn, NULL); >> if (ret < 0) { >> monitor_printf(mon, "Error while creating snapshot on '%s'\n", >> bdrv_get_device_name(bs1));