Re: [Qemu-devel] [PATCH v2 09/20] cow: correctly propagate errors

[Markus Armbruster]

Jeff Cody <address@hidden> writes:

> On Fri, Feb 14, 2014 at 05:45:40PM +0100, Kevin Wolf wrote:
>> Am 11.02.2014 um 18:03 hat Paolo Bonzini geschrieben:
>> > Signed-off-by: Paolo Bonzini <address@hidden>
>> > ---
>> >  block/cow.c | 12 +++---------
>> >  1 file changed, 3 insertions(+), 9 deletions(-)
>> > 
>> > diff --git a/block/cow.c b/block/cow.c
>> > index 7fc0b12..43a2150 100644
>> > --- a/block/cow.c
>> > +++ b/block/cow.c
>> > @@ -82,7 +82,7 @@ static int cow_open(BlockDriverState *bs, QDict 
>> > *options, int flags,
>> >          char version[64];
>> >          snprintf(version, sizeof(version),
>> >                 "COW version %d", cow_header.version);
>> > -        qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
>> > +        error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
>> >              bs->device_name, "cow", version);
>> >          ret = -ENOTSUP;
>> >          goto fail;
>> > @@ -330,7 +330,6 @@ static int cow_create(const char *filename, 
>> > QEMUOptionParameter *options,
>> >      struct stat st;
>> >      int64_t image_sectors = 0;
>> >      const char *image_filename = NULL;
>> > -    Error *local_err = NULL;
>> >      int ret;
>> >      BlockDriverState *cow_bs;
>> >  
>> > @@ -344,18 +343,13 @@ static int cow_create(const char *filename, 
>> > QEMUOptionParameter *options,
>> >          options++;
>> >      }
>> >  
>> > -    ret = bdrv_create_file(filename, options, &local_err);
>> > +    ret = bdrv_create_file(filename, options, errp);
>> >      if (ret < 0) {
>> > -        qerror_report_err(local_err);
>> > -        error_free(local_err);
>> >          return ret;
>> >      }
>> >  
>> > -    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR,
>> > -                         &local_err);
>> > +    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, 
>> > errp);
>> >      if (ret < 0) {
>> > -        qerror_report_err(local_err);
>> > -        error_free(local_err);
>> >          return ret;
>> >      }
>> 
>> This is technically correct, but I think general policy is that using
>> the local_err pattern is preferred anyway.
>>
>
> If I recall correct, I think there are several places that pass errp
> along.  How about this for a rule of thumb policy: use the local_err
> method if the function does not indicate error outside of the passed
> Error pointer.

Use &local_err when you need to examine the error object.  Passing errp
directly is no good then, because it may be null.

When you're forwarding errors without examining them, then passing errp
directly is just fine.