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
> ---
> 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
(As always, I'll rely on block maintainers to do more serious structural
review for cases I am not aware of)
