Am 29.06.2017 um 14:07 hat Manos Pitsidianakis geschrieben:
On Thu, Jun 29, 2017 at 01:18:24PM +0200, Kevin Wolf wrote:
>Am 29.06.2017 um 08:03 hat Manos Pitsidianakis geschrieben:
>>bdrv_open_driver() is called in two places, bdrv_new_open_driver() and
>>bdrv_open_common(). In the latter, failure cleanup in is in its caller,
>>bdrv_open_inherit(), which unrefs the bs->file of the failed driver open
>>if it exists. Let's check for this in bdrv_new_open_driver() as well.
>>
>>Signed-off-by: Manos Pitsidianakis <address@hidden>
>>---
>> block.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>>diff --git a/block.c b/block.c
>>index 694396281b..aeacd520e0 100644
>>--- a/block.c
>>+++ b/block.c
>>@@ -1165,6 +1165,9 @@ BlockDriverState *bdrv_new_open_driver(BlockDriver
*drv, const char *node_name,
>>
>> ret = bdrv_open_driver(bs, drv, node_name, bs->options, flags, errp);
>> if (ret < 0) {
>>+ if (bs->file != NULL) {
>>+ bdrv_unref_child(bs, bs->file);
>>+ }
>> QDECREF(bs->explicit_options);
>> QDECREF(bs->options);
>> bdrv_unref(bs);
>
>I think we should set bs->file = NULL here to remove the dangling
>pointer. I think it is never accessed anyway because of the
>bs->drv = NULL in the error path of bdrv_open_driver(), but better safe
>than sorry.
You can't see it in the diff but after bdrv_unref(bs),
bdrv_new_open_driver returns NULL so there won't be any access to bs
anyway. And since bs is destroyed by bdrv_unref (its refcount is 1),
there's not really a point in setting bs->file = NULL.
Yes, but bdrv_unref() doesn't have to expect inconsistent BDSes. It
doesn't access bs->file currently when bs->drv == NULL, but that's more
by luck than by design.
>But what would you think about avoiding the code duplication and just
>moving the bdrv_unref_child() call from bdrv_open_inherit() down to
>bdrv_open_driver(), so that bdrv_new_open_driver() is automatically
>covered?
The result would be the same, but this will cover future callers of
bdrv_open_driver. Should I submit a v2?
I would prefer this, yes.
signature.asc