From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49966) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZZsS7-0003XB-Ti for qemu-devel@nongnu.org; Wed, 09 Sep 2015 23:23:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZZsS3-0005qK-QM for qemu-devel@nongnu.org; Wed, 09 Sep 2015 23:23:11 -0400 References: <1437414365-11881-1-git-send-email-mreitz@redhat.com> <1437414365-11881-30-git-send-email-mreitz@redhat.com> <55EEA69E.4010706@cn.fujitsu.com> <55EF5133.2040807@redhat.com> <55F00363.5080709@cn.fujitsu.com> <55F02D3D.6020102@redhat.com> From: Wen Congyang Message-ID: <55F0F783.6020408@cn.fujitsu.com> Date: Thu, 10 Sep 2015 11:22:43 +0800 MIME-Version: 1.0 In-Reply-To: <55F02D3D.6020102@redhat.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 29/38] blockdev: Add blockdev-insert-medium List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-block@nongnu.org Cc: Kevin Wolf , Alberto Garcia , qemu-devel@nongnu.org, Markus Armbruster , Stefan Hajnoczi , John Snow On 09/09/2015 08:59 PM, Max Reitz wrote: > On 09.09.2015 12:01, Wen Congyang wrote: >> On 09/09/2015 05:20 AM, Max Reitz wrote: >>> On 08.09.2015 11:13, Wen Congyang wrote: >>>> On 07/21/2015 01:45 AM, 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 | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ >>>>> qapi/block-core.json | 17 +++++++++++++++++ >>>>> qmp-commands.hx | 37 +++++++++++++++++++++++++++++++++++++ >>>>> 3 files changed, 102 insertions(+) >>>>> >>>>> diff --git a/blockdev.c b/blockdev.c >>>>> index 481760a..a80d0e2 100644 >>>>> --- a/blockdev.c >>>>> +++ b/blockdev.c >>>>> @@ -2164,6 +2164,54 @@ void qmp_blockdev_remove_medium(const char *device, Error **errp) >>>>> } >>>>> } >>>>> >>>>> +static void qmp_blockdev_insert_anon_medium(const char *device, >>>>> + BlockDriverState *bs, Error **errp) >>>>> +{ >>>>> + BlockBackend *blk; >>>>> + bool has_device; >>>>> + >>>>> + blk = blk_by_name(device); >>>>> + if (!blk) { >>>>> + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, >>>>> + "Device '%s' not found", device); >>>>> + return; >>>>> + } >>>>> + >>>>> + /* For BBs without a device, we can exchange the BDS tree at will */ >>>>> + has_device = blk_get_attached_dev(blk); >>>>> + >>>>> + if (has_device && !blk_dev_has_removable_media(blk)) { >>>>> + error_setg(errp, "Device '%s' is not removable", device); >>>>> + return; >>>>> + } >>>>> + >>>>> + if (has_device && !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; >>>>> + } >>>>> + >>>>> + blk_insert_bs(blk, bs); >>>>> +} >>>>> + >>>>> +void qmp_blockdev_insert_medium(const char *device, const char *node_name, >>>>> + Error **errp) >>>>> +{ >>>>> + BlockDriverState *bs; >>>>> + >>>>> + bs = bdrv_find_node(node_name); >>>>> + if (!bs) { >>>>> + error_setg(errp, "Node '%s' not found", node_name); >>>>> + return; >>>>> + } >>>> >>>> Hmm, it is OK if the bs is not top BDS? >>> >>> I think so, yes. Generally, there's probably no reason to do that, but I >>> don't know why we should not allow that case. For instance, you might >>> want to make a backing file available read-only somewhere. >>> >>> It should be impossible to make it available writable, and it should not >>> be allowed to start a block-commit operation while the backing file can >>> be accessed by the guest, but this should be achieved using op blockers. >>> >>> What we need for this to work are fine-grained op blockers, I think. But >>> working around that for now by only allowing to insert top BDS won't >>> work, since you can still start block jobs which target top BDS, too >>> (e.g. blockdev-backup can write to a BDS/BB that is visible to the guest). >>> >>> All in all, I think it's fine to insert non-top BDS, but we should >>> definitely worry about which exact BDS one can insert once we have >>> fine-grained op blockers. >> >> A BDS can be written by its parent, its block backend, a block job.. >> So I think we should have some way to avoid more than two sources writing >> to it, otherwise the data may be corrupted. > > Yes, and that would be op blockers. > > As I said, using blockdev-backup you can write to a BB that can be > written to by the guest as well. I think this is a bug, but it is a bug > that needs to be fixed by having better op blockers in place, which Jeff > Cody is working on. I don't find such patches in the maillist. Thanks Wen Congyang > > Regarding this series, I don't consider this to be too big of an issue. > Yes, if you are working with floppy disks, you can have the case of a > block job and the guest writing to the BDS at the same time. But I can't > really imagine who would use floppy disks and block jobs at the same > time (people who still use floppy disks for their VMs don't strike me as > the kind of people who use the management features of qemu, especially > not for those floppy disks). > > Other than that, this function (blockdev-insert-medium) can only be used > for optical ROM devices (I don't think we have CD/DVD-RW support, do > we?), so it's much less of an issue there. > > So all in all I don't consider this too big of an issue here. If others > think different, then I would delay this part of the series (which > overhauls the "change" command) until we have fine-grained op blockers. > > Max >