From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42307) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Za7EH-0000K1-30 for qemu-devel@nongnu.org; Thu, 10 Sep 2015 15:09:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Za7EF-0003Tc-Ry for qemu-devel@nongnu.org; Thu, 10 Sep 2015 15:09:53 -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> <55F0D901.1010607@cn.fujitsu.com> From: Max Reitz Message-ID: <55F1D575.9020909@redhat.com> Date: Thu, 10 Sep 2015 21:09:41 +0200 MIME-Version: 1.0 In-Reply-To: <55F0D901.1010607@cn.fujitsu.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="PiEP1lJUWrNstaOKw83kpaT9s3kbIG7uX" 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: Wen Congyang , qemu-block@nongnu.org Cc: Kevin Wolf , Alberto Garcia , qemu-devel@nongnu.org, Markus Armbruster , Stefan Hajnoczi , John Snow This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --PiEP1lJUWrNstaOKw83kpaT9s3kbIG7uX Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 10.09.2015 03:12, Wen Congyang wrote: > 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 fo= r >>>>>> 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) >>>>>> } >>>>>> } >>>>>> =20 >>>>>> +static void qmp_blockdev_insert_anon_medium(const char *device, >>>>>> + BlockDriverState *bs,= Error **errp) >>>>>> +{ >>>>>> + BlockBackend *blk; >>>>>> + bool has_device; >>>>>> + >>>>>> + blk =3D 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 =3D 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", devic= e); >>>>>> + 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 *n= ode_name, >>>>>> + Error **errp) >>>>>> +{ >>>>>> + BlockDriverState *bs; >>>>>> + >>>>>> + bs =3D 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, b= ut I >>>> don't know why we should not allow that case. For instance, you migh= t >>>> 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 block= ers. >>>> >>>> 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 g= uest). >>>> >>>> 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 wri= ting >>> 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 bu= g >> that needs to be fixed by having better op blockers in place, which Je= ff >> Cody is working on. >> >> Regarding this series, I don't consider this to be too big of an issue= =2E >> 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 us= ed >> 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 other= s >> think different, then I would delay this part of the series (which >> overhauls the "change" command) until we have fine-grained op blockers= =2E >=20 > In most cases, the user uses this command to change CD/DVD media, so it= is OK. > But IIRC scsi disk can also be changed. So we can mark this command as = experimental > (the command name can be x-blockdev-insert-medium). I'd rather delay this part than mark it experimental. But then again, seeing that we have cases like this already (i.e. blockdev-backup) and nobody seems to be complaining, I still think it should be fine. Max --PiEP1lJUWrNstaOKw83kpaT9s3kbIG7uX Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJV8dV1AAoJEDuxQgLoOKytF+EIAI7iub0e/UVaBc1x9heB3vBz dwwmnW0RDGkgNwZvG3yxOIAOB9oQe1nQ2PmlgD2fPfWEO4WmuKCgplym1XuxrQQz lh0u5ZvDyR/vNCx0xlGzB+9B44GzLAaEXM9H/9TkuUSHuH/jqJWvZXWTwO52hI6n P/8f7GDRVqfU9BGE+RmQHJzMLT0Qt5VNAbS/tdkLOX9JlTn3nYUwS7eqJ4i07Q3w BKU+nwnz8PN/pMGxN+KV21sA5NmoyUyDyEhZkBufjqUkLOdO5Mkvk356ruE+POwf VsqWi3TU8RIreULZKcaNP9CH8NqrK4rs/lFmCuc9AuRnu188p0vOcR2ml9Riz+k= =Wuqk -----END PGP SIGNATURE----- --PiEP1lJUWrNstaOKw83kpaT9s3kbIG7uX--