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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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