| Message ID | 1447127063-4662-4-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue 10 Nov 2015 04:44:18 AM CET, Max Reitz wrote: > -int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) > +static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs, > + int flags, Error **errp) > { > /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */ > char *tmp_filename = g_malloc0(PATH_MAX + 1); > @@ -1354,11 +1355,15 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) > goto out; > } > > + bdrv_ref(bs_snapshot); > bdrv_append(bs_snapshot, bs); > > + g_free(tmp_filename); > + return bs_snapshot; > + > out: > g_free(tmp_filename); > - return ret; > + return NULL; > } If I'm not wrong, now that you're not returning 'ret' anymore there's a "ret = total_size" line earlier in this function that is useless now. Other than that, Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
On 01.12.2015 15:35, Alberto Garcia wrote: > On Tue 10 Nov 2015 04:44:18 AM CET, Max Reitz wrote: >> -int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) >> +static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs, >> + int flags, Error **errp) >> { >> /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */ >> char *tmp_filename = g_malloc0(PATH_MAX + 1); >> @@ -1354,11 +1355,15 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) >> goto out; >> } >> >> + bdrv_ref(bs_snapshot); >> bdrv_append(bs_snapshot, bs); >> >> + g_free(tmp_filename); >> + return bs_snapshot; >> + >> out: >> g_free(tmp_filename); >> - return ret; >> + return NULL; >> } > > If I'm not wrong, now that you're not returning 'ret' anymore there's a > "ret = total_size" line earlier in this function that is useless now. Yes, indeed, thanks for finding that. Will fix. Max > Other than that, > > Reviewed-by: Alberto Garcia <berto@igalia.com> > > Berto >
diff --git a/block.c b/block.c index aae1d69..c5ea5e7 100644 --- a/block.c +++ b/block.c @@ -1295,7 +1295,8 @@ done: return c; } -int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) +static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs, + int flags, Error **errp) { /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */ char *tmp_filename = g_malloc0(PATH_MAX + 1); @@ -1354,11 +1355,15 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) goto out; } + bdrv_ref(bs_snapshot); bdrv_append(bs_snapshot, bs); + g_free(tmp_filename); + return bs_snapshot; + out: g_free(tmp_filename); - return ret; + return NULL; } /* @@ -1557,15 +1562,29 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename, } QDECREF(options); - *pbs = bs; /* For snapshot=on, create a temporary qcow2 overlay. bs points to the * temporary snapshot afterwards. */ if (snapshot_flags) { - ret = bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err); + BlockDriverState *snapshot_bs; + snapshot_bs = bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err); if (local_err) { + ret = -EINVAL; goto close_and_fail; } + if (!*pbs) { + /* The reference is now held by the overlay BDS */ + bdrv_unref(bs); + bs = snapshot_bs; + } else { + /* It is still referenced in the same way that *pbs was referenced, + * however that may be */ + bdrv_unref(snapshot_bs); + } + } + + if (!*pbs) { + *pbs = bs; } return 0; diff --git a/include/block/block.h b/include/block/block.h index 1abfc70..37e49a1 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -210,7 +210,6 @@ BdrvChild *bdrv_open_child(const char *filename, bool allow_none, Error **errp); void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd); int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp); -int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp); int bdrv_open(BlockDriverState **pbs, const char *filename, const char *reference, QDict *options, int flags, Error **errp); BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
If bdrv_open_inherit() creates a snapshot BDS and *pbs is NULL, that snapshot BDS should be returned instead of the BDS under it. To this end, bdrv_append_temp_snapshot() now returns the snapshot BDS instead of just appending it on top of the snapshotted BDS, and that function is made static since bdrv_open_inherit() is its only user anyway. Also, it calls bdrv_ref() before bdrv_append() (which bdrv_open_inherit() has to undo if not returning the overlay). Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 27 +++++++++++++++++++++++---- include/block/block.h | 1 - 2 files changed, 23 insertions(+), 5 deletions(-)