From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:41553) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QwbI2-0006Sy-I7 for qemu-devel@nongnu.org; Thu, 25 Aug 2011 10:52:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QwbI1-0007tS-Io for qemu-devel@nongnu.org; Thu, 25 Aug 2011 10:52:18 -0400 Received: from mx1.redhat.com ([209.132.183.28]:64054) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QwbI1-0007tB-7v for qemu-devel@nongnu.org; Thu, 25 Aug 2011 10:52:17 -0400 Date: Thu, 25 Aug 2011 11:52:08 -0300 From: Luiz Capitulino Message-ID: <20110825115208.0294613a@doriath> In-Reply-To: <4E565A4E.2050603@us.ibm.com> References: <1314211389-28915-1-git-send-email-aliguori@us.ibm.com> <1314211389-28915-13-git-send-email-aliguori@us.ibm.com> <20110825110957.230464dd@doriath> <4E565A4E.2050603@us.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 12/14] qapi: introduce change-blockdev List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Kevin Wolf , qemu-devel@nongnu.org, Michael Roth On Thu, 25 Aug 2011 09:21:02 -0500 Anthony Liguori wrote: > On 08/25/2011 09:09 AM, Luiz Capitulino wrote: > > On Wed, 24 Aug 2011 13:43:07 -0500 > > Anthony Liguori wrote: > > > >> A new QMP only command to change the blockdev associated with a block device. > >> The semantics of change right now are just plain scary. This command introduces > >> sane semantics. For more details, see the documentation in the schema file. > > > > IMO, this has to be split into two commits. First you introduce the new command > > and then you fix do_change_block(). > > > > Also, there's a small problem with the generated code: it has to return -1 on > > errors. The monitor expects a -1 return _and_ a qerror object on errors. The > > generated code in middle mode is returning 0 even on errors, which makes QMP to > > emit a UndefinedError error by default. > > You mean: > > if (local_err) { > qerror_report_err(local_err); > error_free(local_err); > return -1; > } > return 0; > > That should behave exactly the right way as qerror_report_err() > propagates sends the Error as a QError. Oh that's right, what's happening is that qerror_report_err() is returning before setting qerror in the Monitor object... I reported that in my review of patch 01/14. > > >> + > >> +## > >> +# @change-blockdev: > >> +# > >> +# This is the preferred interface for changing a block device. > >> +# > >> +# @device: The block device name. > >> +# > >> +# @filename: The new filename for the block device. This may contain options > >> +# encoded in a format specified by @format. > >> +# > >> +# @format: #optional The format to use open the block device > > > > We need a list (or a pointers) of valid formats. > > We do, but that needs to be a command because it depends on the build > options. > > >> +# > >> +# @password: #optional The password to use if the block device is encrypted > >> +# > >> +# Returns: Nothing on success. > >> +# If @device is not a valid block device, DeviceNotFound > >> +# If @format is not a valid block format, InvalidBlockFormat > >> +# If @filename is encrypted and @password isn't supplied, > >> +# DeviceEncrypted. The call should be repeated with @password > >> +# supplied. > >> +# If @format is not specified and @filename is a format that cannot > >> +# be safely probed, MissingParameter. > >> +# If @filename cannot be opened, OpenFileFailed > >> +# > >> +# Since: 1.0 > >> +## > >> +{ 'command': 'change-blockdev', > >> + 'data': {'device': 'str', 'filename': 'str', '*format': 'str', > >> + '*password': 'str'} } > >> diff --git a/qmp-commands.hx b/qmp-commands.hx > >> index c0d0ca3..cec7135 100644 > >> --- a/qmp-commands.hx > >> +++ b/qmp-commands.hx > >> @@ -121,6 +121,14 @@ EQMP > >> .mhandler.cmd_new = do_change, > >> }, > >> > >> + { > >> + .name = "change-blockdev", > >> + .args_type = "device:B,target:F,arg:s?pass:s?", > > > > The arguments names don't match the schema. > > Do we need to set args_type for QMP? Yes. > > Regards, > > Anthony Liguori > > > >> + .params = "device filename [format] [password]", > >> + .help = "change a removable medium, optional format", > >> + .mhandler.cmd_new = qmp_marshal_input_change_blockdev, > >> + }, > >> + > >> SQMP > >> change > >> ------ > > >