Re: [Qemu-devel] [PATCH 02/18] block: add error parameter to bdrv_snapshot_create() and related functions

[Luiz Capitulino]

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.
>