From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:54217) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QmQ14-0004bw-Dq for qemu-devel@nongnu.org; Thu, 28 Jul 2011 08:48:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QmQ13-00037N-9K for qemu-devel@nongnu.org; Thu, 28 Jul 2011 08:48:42 -0400 Received: from mail-yi0-f45.google.com ([209.85.218.45]:42789) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QmQ13-00037G-3M for qemu-devel@nongnu.org; Thu, 28 Jul 2011 08:48:41 -0400 Received: by yia25 with SMTP id 25so2115547yia.4 for ; Thu, 28 Jul 2011 05:48:40 -0700 (PDT) Message-ID: <4E315AA6.4070803@codemonkey.ws> Date: Thu, 28 Jul 2011 07:48:38 -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> <4E31365F.2000904@in.ibm.com> In-Reply-To: <4E31365F.2000904@in.ibm.com> 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: Supriya Kannery Cc: Kevin Wolf , Stefan Hajnoczi , Christoph Hellwig , Supriya Kannery , qemu-devel@nongnu.org On 07/28/2011 05:13 AM, Supriya Kannery wrote: > On 07/27/2011 09:32 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. >> > > User can programmatically find out valid parameters for > block_set. Internally, validation of prop=value is done against -drive > parameter list and then, only the valid/implemented commands (for now, > hostcache) are accepted from that list. Please find execution output > attached: > ======================================================================== > (qemu) info block > ide0-hd0: removable=0 hostcache=1 file=../rhel6-32.qcow2 ro=0 drv=qcow2 > encrypted=0 > floppy0: removable=1 locked=0 hostcache=0 file=test.img ro=0 drv=raw > encrypted=0 > ide1-cd0: removable=1 locked=0 hostcache=1 [not inserted] > sd0: removable=1 locked=0 hostcache=1 [not inserted] > (qemu) block > block_resize block_set block_passwd > (qemu) block_set That's HMP btw, not QMP. > block_set: block device name expected > (qemu) block > block_resize block_set block_passwd > (qemu) help block_set > block_set device [prop=value][,...] -- Change block device parameters > [hostcache=on/off] Parsing help text is not introspection! Regards, Anthony Liguori > (qemu) block_set ide > Device 'ide' not found > (qemu) block_set ide0-hd0 > Usage: block_set device [prop=value][,...] > (qemu) block_set ide0-hd0 cache > Invalid parameter 'cache' > (qemu) block_set ide0-hd0 cache=on > Parameter 'hostcache' expects on/off > (qemu) block_set ide0-hd0 hostcache=on > (qemu) block_set ide0-hd0 hostcache=off > (qemu) info block > ide0-hd0: removable=0 hostcache=0 file=../rhel6-32.qcow2 ro=0 drv=qcow2 > encrypted=0 > floppy0: removable=1 locked=0 hostcache=0 file=test.img ro=0 drv=raw > encrypted=0 > ide1-cd0: removable=1 locked=0 hostcache=1 [not inserted] > sd0: removable=1 locked=0 hostcache=1 [not inserted] > ======================================================================== > > When we add further parameters, "usage" string can be enhanced to > include those parameters for informing user. > > - Thanks, Supriya > >> OTOH, if you have strong types like block_set_hostcache, query-commands >> tells me exactly what's supported. >> >> Regards, >> >> Anthony Liguori >> >>> >>> Stefan >>> >> >> > >