| Message ID | 20200316060631.30052-3-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | zero pointer after bdrv_unref_child | expand |
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 <vsementsov@virtuozzo.com> > --- > 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 <jsnow@redhat.com> (As always, I'll rely on block maintainers to do more serious structural review for cases I am not aware of)
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; } 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; } qcow2_refcount_close(bs);
data_file being NULL doesn't seem to be a correct state, but it's better than dead pointer and simpler to debug. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/qcow2.c | 2 ++ 1 file changed, 2 insertions(+)