Re: [PATCH 2/2] block/qcow2: zero data_file child after free

[John Snow]

On 3/16/20 2:06 AM, Vladimir Sementsov-Ogievskiy wrote:
> data_file being NULL doesn't seem to be a correct state, but it's
> better than dead pointer and simpler to debug.
> 

How important is it to have correct state in the middle of teardown?

> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/qcow2.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index d44b45633d..6cdefe059f 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1758,6 +1758,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
> *bs, QDict *options,
>      g_free(s->image_data_file);
>      if (has_data_file(bs)) {
>          bdrv_unref_child(bs, s->data_file);
> +        s->data_file = NULL;
>      }

Probably OK to set to NULL, since this is at the end of a failed open
where it would have been set for the first time anyway.

It's an invalid state, but resulting from a failed call. I think that's OK.

(Are there any callers of bdrv_open or qcow2_open that don't just
immediately trash this object if it failed? I don't know of any, but
there's a lot of callers to bdrv_open.)

>      g_free(s->unknown_header_fields);
>      cleanup_unknown_header_ext(bs);
> @@ -2621,6 +2622,7 @@ static void qcow2_close(BlockDriverState *bs)
>  
>      if (has_data_file(bs)) {
>          bdrv_unref_child(bs, s->data_file);
> +        s->data_file = NULL;
>      }
>  

Probably fine here too. I can't imagine it's valid to use this object
after close() ... unless we open it again, and that should handle
setting this back to a realistic value.

>      qcow2_refcount_close(bs);
> 



So I think this is fine? If I understand right this just makes failures
more obvious if we do accidentally use this value after a failed open or
close, so that seems fine.

Reviewed-by: John Snow <address@hidden>


(As always, I'll rely on block maintainers to do more serious structural
review for cases I am not aware of)