[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 02/18] block: add error parameter to bdrv_snapsh
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH 02/18] block: add error parameter to bdrv_snapshot_create() and related functions |
Date: |
Fri, 31 Aug 2012 10:09:47 -0300 |
On Fri, 31 Aug 2012 08:26:38 +0200
Markus Armbruster <address@hidden> wrote:
> Luiz Capitulino <address@hidden> writes:
>
> > On Wed, 15 Aug 2012 09:41:43 +0200
> > Pavel Hrdina <address@hidden> wrote:
> >
> >> Signed-off-by: Pavel Hrdina <address@hidden>
> >> ---
> >> 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);
> >
> >> + ret = -ENOMEDIUM;
> >
> > And, usually, we should get rid of errno propagation. There are two cases
> > here:
>
> The block layer consistently[*] uses -errno return values. Its
> consistency is valuable, and I'm a bit reluctant to break it. Maybe a
> new rule "returns -errno, except when it has an Error ** argument" could
> work. I'd like to hear Kevin's advice on this.
>
> >
> > 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.
>
> Let's not reinvent errno, poorly.
>
> > 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.
>
> Better.
>
> What's ErrorClass doing there?
There might be cases where we will have to report an error other than
GenericError,
we could have error_sete_generic(), but this is starting to get weird :)
>
> [...]
>
>
> [*] Almost; it's still QEMU after all.
>
[Qemu-devel] [PATCH 05/18] block: add error parameter to bdrv_snapshot_list() and related functions, Pavel Hrdina, 2012/08/15
[Qemu-devel] [PATCH 04/18] block: add error parameter to bdrv_snapshot_delete() and related functions, Pavel Hrdina, 2012/08/15
[Qemu-devel] [PATCH 06/18] block: add error parameter to bdrv_snapshot_find(), Pavel Hrdina, 2012/08/15
[Qemu-devel] [PATCH 07/18] block: add error parameter to del_existing_snapshots(), Pavel Hrdina, 2012/08/15
[Qemu-devel] [PATCH 03/18] block: add error parameter to bdrv_snapshot_goto() and related functions, Pavel Hrdina, 2012/08/15
[Qemu-devel] [PATCH 08/18] savevm: add error parameter to qemu_savevm_state_begin(), Pavel Hrdina, 2012/08/15
[Qemu-devel] [PATCH 12/18] savevm: add error parameter to qemu_loadvm_state(), Pavel Hrdina, 2012/08/15
[Qemu-devel] [PATCH 11/18] savevm: add error parameter to qemu_savevm_state(), Pavel Hrdina, 2012/08/15