On Fri, 31 Aug 2012 08:26:38 +0200 Markus Armbruster <arm...@redhat.com> wrote:
> Luiz Capitulino <lcapitul...@redhat.com> writes: > > > 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); > > > >> + 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. >