On Mon 11 Jun 2018 02:20:06 PM CEST, Kevin Wolf wrote: > Am 01.06.2018 um 12:51 hat Alberto Garcia geschrieben: >> On Thu 03 May 2018 02:22:41 PM CEST, Kevin Wolf wrote: >> >> > Were the (more or less) exact requirements of QMP blockdev-reopen >> >> > discussed? How is it different from qemu-io's "reopen" command? >> >> > What are the options that you can and can not change? >> >> >> >> I can't quite remember, I'm afraid. I think it was supposed to be >> >> pretty much qemu-io's reopen (so just bdrv_reopen()). I suppose you >> >> cannot change the driver (obviously) or probably the node name, because >> >> either would result in the node being replaced by a completely new one. >> >> >> >> Other than that, it probably depends on what the block driver supports, >> >> but ideally you should be able to change everything. >> > >> > Honestly the design of bdrv_reopen() is quite broken because of the >> > way it tries to maintain old options if they aren't specified, and >> > guesses what you might mean when you add flags to the mix. The exact >> > semantics are quite complicated and I'd rather avoid them in a stable >> > API. >> > >> > A clean QMP command would probably apply the same defaults as >> > blockdev-add, so you just get to specify the full options again. >> >> I have a prototype of this working and almost ready to be published, but >> there's a tricky thing with this part: >> >> If we want blockdev-reopen to apply the defaults for all options except >> from the ones expliclity specified by the user, then it means that we >> need to check not just the options that are present, but also the ones >> that are omitted. >> >> For example: >> >> { "execute": "blockdev-add", >> "arguments": { "driver": "null-aio", >> "node-name": "root", >> "size": 1024 } >> >> This adds a null-aio block device with the "size" option set to 1024 >> (the default is 1 << 30). >> >> null_reopen_prepare() allows reopening that block device, but it does >> not allow changing any of its options. Attempting to change the value of >> "size" is detected by the loop that checks unhandled options at the end >> of bdrv_reopen_prepare() and returns "Cannot change the option 'size'". >> >> So far, so good. We have this generic check for all options that works >> with all drivers, so as long as we only specify options that we know >> that can be changed, everything is fine. >> >> However if we want blockdev-reopen to apply the default values for all >> omitted options, then omitting "size" would be equivalent to setting it >> to its default value (1 << 30). And if "size" cannot be changed then >> QEMU should complain unless we explicitly set "size" to 1024 again on >> reopen. >> >> This complicates things a bit, because we would go from "the options >> that can't be changed are the ones that are not handled by each driver's >> _prepare() function" to "options that are absent can also produce an >> error". > > To be honest, I think this is fine. If the user can specify the size > once (in blockdev-add), they can do it again (in blockdev-reopen). > > We just need to make sure that we don't break existing bdrv_reopen*() > calls that come from places other than the monitor.
Good. I have the first revision of the series almost ready, expect it this week. Berto