Re: [PATCH v7 03/14] block: check return value of bdrv_open_child and drop error propagation

[Vladimir Sementsov-Ogievskiy]

15.02.2021 23:04, Eric Blake wrote:
> On 2/15/21 3:22 AM, Kevin Wolf wrote:
>
> > > With this patch applied, 'check unit-test' fails with:
> > >
> > > Running test test-replication
> > > Unexpected error in bdrv_open_driver() at ../block.c:1481:
> > > Could not open '/tmp/p_local_disk.z1Ugyc': Invalid argument
> > > ERROR test-replication - missing test plan
>
> > The problem is this hunk:
> >
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 5d94f45be9..e8dd42d73b 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -1611,9 +1611,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
> > *bs, QDict *options,
> >       /* Open external data file */
> >       s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
> >                                      &child_of_bds, BDRV_CHILD_DATA,
> > -                                   true, &local_err);
> > -    if (local_err) {
> > -        error_propagate(errp, local_err);
> > +                                   true, errp);
> > +    if (!s->data_file) {
> >           ret = -EINVAL;
> >           goto fail;
> >       }
> >
> > bdrv_open_child() can return NULL in non-error cases, when the child is
> > optional and it isn't given. The test doesn't use an external data file,
> > so this returns NULL without setting an error, which now gets turned
> > into -EINVAL instead.
> >
> > This makes the most basic tests fail with qcow2 (iotests 001 is enough).
> >
> > The other hunks in this patch don't suffer from the same problem because
> > they pass allow_none=false.
>
> Thanks; that's enough to figure out how to repair the patch:
>
> diff --git i/block/qcow2.c w/block/qcow2.c
> index e8dd42d73b4c..38137ca30eb0 100644
> --- i/block/qcow2.c
> +++ w/block/qcow2.c
> @@ -1292,6 +1292,7 @@ static int
> validate_compression_type(BDRVQcow2State *s, Error **errp)
>   static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>                                         int flags, Error **errp)
>   {
> +    ERRP_GUARD();
>       BDRVQcow2State *s = bs->opaque;
>       unsigned int len, i;
>       int ret = 0;
> @@ -1612,7 +1613,7 @@ static int coroutine_fn
> qcow2_do_open(BlockDriverState *bs, QDict *options,
>       s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
>                                      &child_of_bds, BDRV_CHILD_DATA,
>                                      true, errp);
> -    if (!s->data_file) {
> +    if (*errp) {
>           ret = -EINVAL;
>           goto fail;
>       }

Agree.. Or better refactor bdrv_open_child to follow more common (and 
recommended) semantics (i.e. NULL + errp on error, non-null on succsess).. But 
this will require more investigation.

I'm busy now with our internal deadline (this week), after this will return to 
handle upstream things.

--
Best regards,
Vladimir