Am 01.07.2017 um 17:39 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 move the bs->file cleanup to bdrv_open_driver() to take care of all > callers and do not set bs->drv to NULL unless the driver's open function > failed. When bs is destroyed by removing its last reference, bdrv_close() > checks bs->drv to perform the needed cleanups and also call the driver's close > function. > > Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr> > --- > > v2: > move bdrv_unref_child(bs, bs->file) to bdrv_open_driver > do not set bs->drv to NULL if open succeeds > > block.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/block.c b/block.c > index 694396281b..df2a46990c 100644 > --- a/block.c > +++ b/block.c > @@ -1091,6 +1091,7 @@ static int bdrv_open_driver(BlockDriverState *bs, > BlockDriver *drv, > { > Error *local_err = NULL; > int ret; > + bool open_failed; > > bdrv_assign_node_name(bs, node_name, &local_err); > if (local_err) { > @@ -1111,7 +1112,9 @@ static int bdrv_open_driver(BlockDriverState *bs, > BlockDriver *drv, > ret = 0; > } > > - if (ret < 0) { > + open_failed = ret < 0; > + > + if (open_failed) { > if (local_err) { > error_propagate(errp, local_err); > } else if (bs->filename[0]) { > @@ -1142,10 +1145,15 @@ static int bdrv_open_driver(BlockDriverState *bs, > BlockDriver *drv, > return 0; > > free_and_fail: > - /* FIXME Close bs first if already opened*/ > - g_free(bs->opaque); > - bs->opaque = NULL; > - bs->drv = NULL; > + if (open_failed) { > + g_free(bs->opaque); > + bs->opaque = NULL; > + bs->drv = NULL; > + } > + if (bs->file != NULL) { > + bdrv_unref_child(bs, bs->file); > + bs->file = NULL; > + }
Is this bdrv_unref_child() safe if we leave bs->drv set? Format drivers expect that if an image is opened, it also has a valid bs->file. For example, if I add ret = -1 after refresh_total_sectors() (because I couldn't find an easier way to make it fail intentionally), I get an ugly heap corruption crash instead of a nice error message with this patch. Kevin