On Thu, 06 Sep 2012 11:07:54 +0200
Pavel Hrdina <phrd...@redhat.com> wrote:
> On 08/30/2012 04:47 PM, Luiz Capitulino wrote:
> > On Wed, 15 Aug 2012 09:41:43 +0200
> > Pavel Hrdina <phrd...@redhat.com> wrote:
> >
> >> Signed-off-by: Pavel Hrdina <phrd...@redhat.com>
> >> ---
> >> 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));
Hmm, no. The best way to refactor code that use the same error message
like the one above is to move the whole operation (with error handling
included) to a wrapper and use the wrapper instead.
Adding macros for error messages is a Bad Thing because soon or later
someone will create a new macro for a very similar but different error
message (which will lead to macro explosion) or will use an existing
macro that doesn't fit well to the failure being reported.
>
> 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 <rbd/librbd.h>
> >>
> >> @@ -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));
>
