Am 17.09.2012 18:43, schrieb Paolo Bonzini: > Il 07/09/2012 18:11, Kevin Wolf ha scritto: >> I was planning to review it in more detail next week, but I just had a >> quick look. I'm not sure if automatically shutting down the NBD server >> when the guest stops using it is always right (for removable media it >> could even be an eject from the guest), > > Yes, the removable media case could be a bit too eager. Note however > that a guest-triggered eject doesn't do bdrv_close, only a > user-triggered eject does, and that's blocked by bdrv_in_use. > > Luckily removable media are usually not too interesting, so a slightly > suboptimal behavior is okay as long as it does not break the important > use cases---mostly migration without shared storage, where also > uninteresting images have to be mirrored or exposed via NBD. Those > should be covered by bdrv_in_use.
It sounds like it could be acceptable, yes. But what's even the motivation to close the server on bdrv_close? The commit message is a bit... well, not just terse, but even empty. The standard case for closing images is that qemu exits. In this case, the NBD server would automatically exit as well. An interesting case for the NBD server would be when the migration has completed; but do we even get a bdrv_close there? >> but introducing a notifier list >> doesn't look too bad. We can probably use it for other things that are >> currently hardcoded in bdrv_close() with some if statements, like >> disabling I/O throttling, cancelling a block job, etc. > > Yes, though a lot of these could be moved to "filters" and use whatever > filter-specific method is there (e.g. a filter bdrv_close). This > circles back to the question of whether bdrv_close kills filters or only > the base image... Note that after completing the refactoring, we'll only have one combined bdrv_close/delete function and so there won't be BlockDriverStates that are closed. In this case, I think it's quite obvious that not closing the filters wouldn't make any sense. Kevin
