From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37690) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGZ5y-0006i7-DI for qemu-devel@nongnu.org; Wed, 28 Jan 2015 15:20:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YGZ5u-0002Bq-BR for qemu-devel@nongnu.org; Wed, 28 Jan 2015 15:20:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55440) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGZ5u-0002Bg-42 for qemu-devel@nongnu.org; Wed, 28 Jan 2015 15:20:10 -0500 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t0SKK97j017813 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 28 Jan 2015 15:20:09 -0500 Message-ID: <54C94477.5000200@redhat.com> Date: Wed, 28 Jan 2015 15:20:07 -0500 From: Max Reitz MIME-Version: 1.0 References: <1422387983-32153-1-git-send-email-mreitz@redhat.com> <1422387983-32153-42-git-send-email-mreitz@redhat.com> <54C9440D.8020702@redhat.com> In-Reply-To: <54C9440D.8020702@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RESEND 41/50] blockdev: Add blockdev-insert-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 15:18, Eric Blake wrote: > On 01/27/2015 12:46 PM, Max Reitz wrote: >> And a helper function for that which directly takes a pointer to the BDS >> to be inserted instead of its node-name (which will be used for >> implementing 'change' using blockdev-insert-medium). >> >> Signed-off-by: Max Reitz >> --- >> blockdev.c | 43 +++++++++++++++++++++++++++++++++++++++++++ >> qapi/block-core.json | 17 +++++++++++++++++ >> qmp-commands.hx | 38 ++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 98 insertions(+) >> >> +static void qmp_blockdev_insert_anon_medium(const char *device, >> + BlockDriverState *bs, Error **errp) >> +{ >> + BlockBackend *blk; >> + >> + blk = blk_by_name(device); >> + if (!blk) { >> + error_set(errp, QERR_DEVICE_NOT_FOUND, device); >> + return; >> + } >> + >> + if (!blk_dev_has_removable_media(blk)) { >> + error_setg(errp, "Device '%s' is not removable", device); >> + return; >> + } >> + >> + if (!blk_dev_is_tray_open(blk)) { >> + error_setg(errp, "Tray of device '%s' is not open", device); >> + return; >> + } >> + >> + if (blk_bs(blk)) { >> + error_setg(errp, "There already is a medium in device '%s'", device); >> + return; >> + } > Good, you didn't implement hot-swap semantics of replacing an existing > medium (that gets too confusing; so I _like_ that you force the user to > consider all the steps through multiple low-level commands). > > >> + >> +Example (1): > I'll quit pointing it out; but if you clean up the useless (1) in one > patch, do it across the series. Yes, will do. >> + >> +-> { "execute": "blockdev-add", >> + "arguments": { "options": { "id": "backend0", >> + "node-name": "node0", > Why is 'id' needed? Oops, because I forgot removing it after I added the functionality for creating a BDS tree without a BB. I'll remove it, thanks for catching it. Max > Isn't the point of this command sequence to create > a BDS tree that is NOT tied to a BB, and then use insert-medium to make > the association after the fact? We don't need to create a BB named > 'backend0' if we are immediately going to reuse 'node0' in a different > BB (true, we have somewhat anticipated the idea of sharing BDS tree > among multiple BB, but haven't quite turned that on before now). > >> + "driver": "raw", >> + "file": { "driver": "file", >> + "filename": "fedora.iso" } } } } >> + >> +<- { "return": {} } >> + >> +-> { "execute": "blockdev-insert-medium", >> + "arguments": { "device": "ide1-cd0", >> + "node-name": "node0" } } >> + >> +<- { "return": {} } > If you can either explain why you used 'id' in the example, or remove > that parameter, you can add: > Reviewed-by: Eric Blake >