From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:54203) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QnuPh-0006sW-8O for qemu-devel@nongnu.org; Mon, 01 Aug 2011 11:28:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QnuPg-0004B4-4n for qemu-devel@nongnu.org; Mon, 01 Aug 2011 11:28:17 -0400 Received: from mail-pz0-f43.google.com ([209.85.210.43]:45366) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QnuPf-0004B0-V7 for qemu-devel@nongnu.org; Mon, 01 Aug 2011 11:28:16 -0400 Received: by pzk1 with SMTP id 1so12100976pzk.30 for ; Mon, 01 Aug 2011 08:28:14 -0700 (PDT) Message-ID: <4E36C608.2030107@codemonkey.ws> Date: Mon, 01 Aug 2011 10:28:08 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <20110727113000.25109.16204.sendpatchset@skannery> <20110727113045.25109.54866.sendpatchset@skannery> <4E300B8E.2020509@codemonkey.ws> <4E3036AA.8030604@codemonkey.ws> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , Supriya Kannery , qemu-devel@nongnu.org, Christoph Hellwig On 08/01/2011 10:22 AM, Stefan Hajnoczi wrote: > On Thu, Jul 28, 2011 at 10:29 AM, Stefan Hajnoczi wrote: >> On Wed, Jul 27, 2011 at 5:02 PM, Anthony Liguori wrote: >>> On 07/27/2011 09:31 AM, Stefan Hajnoczi wrote: >>>> >>>> On Wed, Jul 27, 2011 at 1:58 PM, Anthony Liguori >>>> 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. Regards, Anthony Liguori > Stefan >