On 09.11.2015 17:04, Kevin Wolf wrote: > Am 04.11.2015 um 19:57 hat Max Reitz geschrieben: >> bdrv_delete() is not very happy about deleting BlockDriverStates with >> dirty bitmaps still attached to them. In the past, we got around that >> very easily by relying on bdrv_close_all() bypassing bdrv_delete(), and >> bdrv_close() simply ignoring that condition. We should fix that by >> releasing all dirty bitmaps in bdrv_close() > > This doesn't sound right. If there was a dirty bitmap, there must be a > user associated with it. Now that we simply free the bitmap, that user > has a dangling pointer.
Well, having an assertion there means that we already assumed that case to be impossible. Even though it isn't, as you yourself describe: > An exception would be if we knew that the only "user" of this bitmap is > the monitor because the monitor doesn't actually maintain its own list > of bitmaps. However, it's doubtful whether bdrv_close() should remove > something that the QMP client added explicitly. So you are proposing that bdrv_close() should fail if there are still dirty bitmaps attached? I don't like that either. The bitmaps are attached to the BDS, that much is exposed over QMP, too. If the BDS is released it's only natural to assume that all its bitmaps are released, too. If you don't want that, you need to make sure that the monitor has a reference to the BDS itself so the user can defer the call to blockdev-del until he's/she's ready. Maybe we need some QMP command to fetch a reference for the monitor for that to be more usable, I don't know. It will work with blockdev-add alone, too, though. >> and drop the assertion in bdrv_delete(). > > Why? It should still hold true. But it does not for user-added bitmaps. Actually, on master, you can't break that assertion by adding a bitmap through QMP, but with the BB and media series, you can. And that's because as of that series, eject will no longer force-call bdrv_close() (which bypasses the assertion althogether) but bdrv_unref() instead (leading to bdrv_delete(), and that gets you the assertion). I'm not really keen on fixing that in that series, though, since I consider leaking not much better than a failed assertion, especially considering I'm trying to actually fix it here anyway. Max
signature.asc
Description: OpenPGP digital signature
