From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33790) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZZeyl-0002ZC-7W for qemu-devel@nongnu.org; Wed, 09 Sep 2015 09:00:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZZeyk-0000lu-0q for qemu-devel@nongnu.org; Wed, 09 Sep 2015 08:59:59 -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> From: Max Reitz Message-ID: <55F02D3D.6020102@redhat.com> Date: Wed, 9 Sep 2015 14:59:41 +0200 MIME-Version: 1.0 In-Reply-To: <55F00363.5080709@cn.fujitsu.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Xh7xlaPfWo3m1633K3HfklO2Vp3fuo35F" 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) --Xh7xlaPfWo3m1633K3HfklO2Vp3fuo35F Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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 th= e >>>> 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 *d= evice, Error **errp) >>>> } >>>> } >>>> =20 >>>> +static void qmp_blockdev_insert_anon_medium(const char *device, >>>> + BlockDriverState *bs, E= rror **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 wi= ll */ >>>> + 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", 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 *nod= e_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, 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 n= ot >> be allowed to start a block-commit operation while the backing file ca= n >> be accessed by the guest, but this should be achieved using op blocker= s. >> >> What we need for this to work are fine-grained op blockers, I think. B= ut >> 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 gue= st). >> >> 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. >=20 > 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 writi= ng > 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. 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 --Xh7xlaPfWo3m1633K3HfklO2Vp3fuo35F 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 iQEbBAEBCAAGBQJV8C09AAoJEDuxQgLoOKyt8A8H+NH7WLcR/igrNO2PBwoELZh6 eVnYKzIAPnLLeNl9iVrP6/MGE/afiD2r44kzfNtUGexm8sIO/8zmwQ5BWykMhDik xK6PZxpEMnM+EeRYGzGqwlknToH9C0sz9HSH7sffz2fMr/qPR7H0jlmxsqBtvF6l 1oVsnsPOSVMtsgQFli/Wo7jBUmbjW3gj9dQF6q7tC3ZZ4MN4RfIlSdNo1Uu38H9D XWufXbdjwfVQyVlhVOmUMgN1kLK9nJ/7ASplKzfqSnUkuZXNSgsfEA0bX+9rRr3z rVdADMqhrcbb9pbN+f2jrPAxYDy+xOrKb05UTTosbzAvseAgC5IGvimL0Oz8oA== =0VBD -----END PGP SIGNATURE----- --Xh7xlaPfWo3m1633K3HfklO2Vp3fuo35F--