On 22.09.2015 16:42, Kevin Wolf wrote: > Am 18.09.2015 um 17:22 hat Max Reitz geschrieben: >> This function associates the given BlockDriverState with the given >> BlockBackend. >> >> Signed-off-by: Max Reitz <mre...@redhat.com> >> Reviewed-by: Eric Blake <ebl...@redhat.com> >> Reviewed-by: Alberto Garcia <be...@igalia.com> >> --- >> block/block-backend.c | 16 ++++++++++++++++ >> include/sysemu/block-backend.h | 1 + >> 2 files changed, 17 insertions(+) >> >> diff --git a/block/block-backend.c b/block/block-backend.c >> index 33145f8..652385e 100644 >> --- a/block/block-backend.c >> +++ b/block/block-backend.c >> @@ -314,6 +314,22 @@ void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend >> *blk) >> } >> >> /* >> + * Associates a new BlockDriverState with @blk. >> + */ >> +void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs) >> +{ >> + if (bs->blk == blk) { >> + return; >> + } >> + >> + assert(!blk->bs); >> + assert(!bs->blk); > > Why is it useful to allow reconnecting a BDS to a BB it's already > connected to? I would have expected that we can assert that this is not > the case.
We can do that, too, there is no use case. It's just that I in principle don't like making things an error that do make sense and can trivially be handled gracefully. But I can see people wanting to hit an assertion as soon as something looks fishy. So I'm fine either way. I think Eric asked about the same before, so that makes two against one now. :-) >> + bdrv_ref(bs); >> + blk->bs = bs; >> + bs->blk = blk; >> +} > > My series to remove bdrv_swap() introduces a blk_set_bs() function, > which looks suspiciously similar to this one, except that it allows > passing a BB that already had a BDS (it gets unrefed then) and that I > don't assert that the BDS isn't attached to a BB yet (I should do that). > > Do you think that's similar enough to have only one function? Well, yours looks like blk_remove_bs()+blk_insert_bs() combined. In case we have called blk_remove_bs() before, it will effectively be the same, yes. But that difference still bothers me a bit... I'd prefer implementing blk_set_bs() by calling blk_remove_bs() and then blk_insert_bs() instead, but since these functions are not available in your bdrv_swap() series, that would prove rather difficult. I don't know. Maybe implement it separately for now, and trust that this series will stay in flight long enough for the bdrv_swap() series to get merged so I can include a commit cleaning up blk_set_bs() in this series later on? Or I can base this series on your bdrv_swap() series. Your call, as I haven't looked at it yet and thus don't know how long it'll take to get merged (albeit judging from your series in the past, it won't be too long). Max
signature.asc
Description: OpenPGP digital signature