On Mon, Aug 1, 2011 at 4:44 PM, Kevin Wolf <kw...@redhat.com> wrote: > Am 01.08.2011 17:28, schrieb Anthony Liguori: >> On 08/01/2011 10:22 AM, Stefan Hajnoczi wrote: >>> On Thu, Jul 28, 2011 at 10:29 AM, Stefan Hajnoczi<stefa...@gmail.com> >>> wrote: >>>> On Wed, Jul 27, 2011 at 5:02 PM, Anthony Liguori<anth...@codemonkey.ws> >>>> wrote: >>>>> On 07/27/2011 09:31 AM, Stefan Hajnoczi wrote: >>>>>> >>>>>> On Wed, Jul 27, 2011 at 1:58 PM, Anthony Liguori<anth...@codemonkey.ws> >>>>>> wrote: >>>>>>>> >>>>>>>> Index: qemu/hmp-commands.hx >>>>>>>> =================================================================== >>>>>>>> --- qemu.orig/hmp-commands.hx >>>>>>>> +++ qemu/hmp-commands.hx >>>>>>>> @@ -70,6 +70,20 @@ but should be used with extreme caution. >>>>>>>> resizes image files, it can not resize block devices like LVM >>>>>>>> volumes. >>>>>>>> ETEXI >>>>>>>> >>>>>>>> + { >>>>>>>> + .name = "block_set", >>>>>>>> + .args_type = "device:B,device:O", >>>>>>>> + .params = "device [prop=value][,...]", >>>>>>>> + .help = "Change block device parameters >>>>>>>> [hostcache=on/off]", >>>>>>>> + .user_print = monitor_user_noop, >>>>>>>> + .mhandler.cmd_new = do_block_set, >>>>>>>> + }, >>>>>>>> +STEXI >>>>>>>> +@item block_set @var{config} >>>>>>>> +@findex block_set >>>>>>>> +Change block device parameters (eg: hostcache=on/off) while guest is >>>>>>>> running. >>>>>>>> +ETEXI >>>>>>>> + >>>>>>> >>>>>>> block_set_hostcache() please. >>>>>>> >>>>>>> Multiplexing commands is generally a bad idea. It weakens typing. In >>>>>>> the >>>>>>> absence of a generic way to set block device properties, implementing >>>>>>> properties as generic in the QMP layer seems like a bad idea to me. >>>>>> >>>>>> The idea behind block_set was to have a unified interface for changing >>>>>> block device parameters at runtime. This prevents us from reinventing >>>>>> new commands from scratch. For example, block I/O throttling is >>>>>> already queued up to add run-time parameters. >>>>>> >>>>>> Without a unified command we have a bulkier QMP/HMP interface, >>>>>> duplicated code, and possibly inconsistencies in syntax between the >>>>>> commands. Isn't the best way to avoid these problems a unified >>>>>> interface? >>>>>> >>>>>> I understand the lack of type safety concern but in this case we >>>>>> already have to manually pull parsed arguments (i.e. cast to specific >>>>>> types and deal with invalid input). To me this is a reason *for* >>>>>> using a unified interface like block_set. >>>>> >>>>> Think about it from a client perspective. How do I determine which >>>>> properties are supported by this version of QEMU? I have no way to >>>>> identify >>>>> programmatically what arguments are valid for block_set. >>>>> >>>>> OTOH, if you have strong types like block_set_hostcache, query-commands >>>>> tells me exactly what's supported. >>>> >>>> Use query-block and see if 'hostcache' is there. If yes, then the >>>> hostcache parameter is available. If we allow BlockDrivers to have >>>> their own runtime parameters then query-commands does not tell you >>>> anything because the specific BlockDriver may or may not support that >>>> runtime parameter - you need to use query-block. >>> >>> Let's reach agreement here. The choices are: >>> >>> 1. Top-level block_set command. Supported parameters are discovered >>> by looking query-block output. >> >> I'm strongly opposed to this. There needs to be a single consistent way >> to determine supported operations with QMP. >> >> And that single mechanism already exists--query_commands. >> >>> 2. Top-level command for each parameter (e.g. block_set_hostcache). >>> Supported parameters are easily discoverable via query-commands. If >>> individual block devices support different sets of parameters then >>> they may have to return -ENOTSUPP. >>> >>> I like the block_set approach. >>> >>> Anthony, Kevin, Supriya: Any thoughts? >> >> For the sake of overall QMP sanity, I think block_set_hostcache is >> really our only option. > > Ideally we should have blockdev_add, and blockdev_set would just take > the same arguments and update the given driver. > > But we don't have blockdev_add today, so whatever works for your as a > temporary solution...
Anthony's point is that blockdev_set does not fit with QMP command discoverability. Stefan