| Message ID | 1442589793-7105-22-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <mreitz@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Alberto Garcia <berto@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. > + 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? Kevin
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 <mreitz@redhat.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> Reviewed-by: Alberto Garcia <berto@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
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); + bdrv_ref(bs); + blk->bs = bs; + bs->blk = blk; +} + +/* * Attach device model @dev to @blk. * Return 0 on success, -EBUSY when a device model is attached already. */ diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index 52e35a1..9306a52 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -72,6 +72,7 @@ BlockBackend *blk_by_name(const char *name); BlockBackend *blk_next(BlockBackend *blk); BlockDriverState *blk_bs(BlockBackend *blk); +void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs); void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend *blk);