From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55176) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGa1e-0003CM-2u for qemu-devel@nongnu.org; Wed, 28 Jan 2015 16:19:55 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YGa1c-0003JG-Uv for qemu-devel@nongnu.org; Wed, 28 Jan 2015 16:19:50 -0500 Received: from mx1.redhat.com ([209.132.183.28]:57739) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGa1c-0003J1-MY for qemu-devel@nongnu.org; Wed, 28 Jan 2015 16:19:48 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t0SLJl9Y009668 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 28 Jan 2015 16:19:47 -0500 Message-ID: <54C95271.5090203@redhat.com> Date: Wed, 28 Jan 2015 16:19:45 -0500 From: Max Reitz MIME-Version: 1.0 References: <1422387983-32153-1-git-send-email-mreitz@redhat.com> <1422387983-32153-46-git-send-email-mreitz@redhat.com> <54C94E0F.1080000@redhat.com> In-Reply-To: <54C94E0F.1080000@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RESEND 45/50] qmp: Introduce blockdev-change-medium List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Kevin Wolf , Fam Zheng , Jeff Cody , Markus Armbruster , Stefan Hajnoczi , John Snow On 2015-01-28 at 16:01, Eric Blake wrote: > On 01/27/2015 12:46 PM, Max Reitz wrote: >> Introduce a new QMP command 'blockdev-change-medium' which is intended >> to replace the 'change' command for block devices. The existing function >> qmp_change_blockdev() is accordingly renamed to >> qmp_blockdev_change_medium(). >> >> Signed-off-by: Max Reitz >> --- >> blockdev.c | 7 ++++--- >> include/sysemu/blockdev.h | 2 -- >> qapi-schema.json | 3 ++- >> qapi/block-core.json | 23 +++++++++++++++++++++++ >> qmp-commands.hx | 31 +++++++++++++++++++++++++++++++ >> qmp.c | 2 +- >> 6 files changed, 61 insertions(+), 7 deletions(-) >> >> +++ b/qapi-schema.json >> @@ -1649,7 +1649,8 @@ >> # device between when these calls are executed is undefined. >> # >> # Notes: It is strongly recommended that this interface is not used especially >> -# for changing block devices. >> +# for changing block devices. Please use blockdev-change-medium >> +# instead (for VNC, please use change-vnc-password). > Not grammatically wrong, but still feels a bit awkward. Maybe better > worded as: > > This interface is deprecated, and it is strongly recommended that you > avoid using it. For changing block devices, use blockdev-change-medium; > for changing VNC parameters, use change-vnc-password. Thanks, I'll change it. >> ## >> +# @blockdev-change-medium: >> +# >> +# Changes the medium inserted into a block device by ejecting the current medium >> +# and loading a new image file which is inserted as the new medium (this command >> +# combines blockdev-open-tray, blockdev-remove-medium, blockdev-insert-medium >> +# and blockdev-close-tray). >> +# >> +# @device: block device name >> +# >> +# @filename: filename of the new image to be loaded >> +# >> +# @format: #optional, format to open the new image with (defaults to the >> +# probed format) >> +# >> +# Since: 2.3 >> +## >> +{ 'command': 'blockdev-change-medium', >> + 'data': { 'device': 'str', >> + 'filename': 'str', >> + '*format': 'str' } } > Intentional that there is no way to specify 'force'? I can live with > that (force is a sledgehammer; and someone that can justify using it can > just use the lower-level functions themselves. No need to bloat the > nice wrapper interface with something that is usually not needed). Yes, I'd rather drop 'force' here. > I'm unclear on whether this command will wait for the tray open to > happen, or if it fails in the middle if the tray was locked and a > request sent to the guest but the guest did not act fast enough to > unlock things. As such, I'm not quite sure if this interface is missing > any parameters. Well, I'd rather call this interface deprecated from the start (it's basically as deprecated as 'change' is, only that 'change' is even worse because it unites multiple different commands) than making it suitable for any situation. There are two points from my perspective: First, this is mainly a replacement for 'change'. Since 'change' did not have a 'force' parameter, I don't see the need to have one here either. Second, we can always add additional parameters later (without breaking compatibility); for instance, in this series still the read-only mode parameter will be added. >> +Examples: >> + >> +1. Change a removable medium >> + >> +-> { "execute": "blockdev-change-medium", >> + "arguments": { "device": "ide1-cd0", >> + "filename": "/srv/images/Fedora-12-x86_64-DVD.iso", > Wow - you're still testing a Fedora 12 image? No, I got that file name from the 'change' example. :-) > I'm a bit reluctant to mark this one reviewed, without more discussion. No problem. I'll have to send a v2 anyway. Max