From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48283) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZaRhd-0002Kj-4D for qemu-devel@nongnu.org; Fri, 11 Sep 2015 13:01:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZaRhY-0000Yx-IH for qemu-devel@nongnu.org; Fri, 11 Sep 2015 13:01:33 -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> <55F1D575.9020909@redhat.com> <55F28308.5030303@cn.fujitsu.com> From: Max Reitz Message-ID: <55F308D7.5010200@redhat.com> Date: Fri, 11 Sep 2015 19:01:11 +0200 MIME-Version: 1.0 In-Reply-To: <55F28308.5030303@cn.fujitsu.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="jGbbKak4q6kiPLGnnmQjd3xubic4OIldd" 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) --jGbbKak4q6kiPLGnnmQjd3xubic4OIldd Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 11.09.2015 09:30, Wen Congyang wrote: > On 09/11/2015 03:09 AM, Max Reitz wrote: >> 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 t= o 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 cha= r *device, Error **errp) >>>>>>>> } >>>>>>>> } >>>>>>>> =20 >>>>>>>> +static void qmp_blockdev_insert_anon_medium(const char *device,= >>>>>>>> + BlockDriverState *b= s, 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 a= t 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", dev= ice); >>>>>>>> + 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 =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 mi= ght >>>>>> want to make a backing file available read-only somewhere. >>>>>> >>>>>> It should be impossible to make it available writable, and it shou= ld not >>>>>> be allowed to start a block-commit operation while the backing fil= e can >>>>>> be accessed by the guest, but this should be achieved using op blo= ckers. >>>>>> >>>>>> What we need for this to work are fine-grained op blockers, I thin= k. 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, t= oo >>>>>> (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.= =2E >>>>> So I think we should have some way to avoid more than two sources w= riting >>>>> 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 iss= ue. >>>> 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 c= an'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 m= e as >>>> the kind of people who use the management features of qemu, especial= ly >>>> 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 oth= ers >>>> think different, then I would delay this part of the series (which >>>> overhauls the "change" command) until we have fine-grained op blocke= rs. >>> >>> 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 a= s 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. >=20 > Hmm, another question, when will you post the newest patchset? Block re= plication is > based on this patchset. The best I can say is "once I have the time". There are a lot of things going on right now, not only regarding code I have to write, but also regarding patches I have to review. This series hasn't really been on other people's priority list for eight months now, so I had to take up other things in between which means that now I cannot just focus on this series alone and don't do anything else. Be assured that I took notice that you requested a new version, though, so I will work on it once I can. Max --jGbbKak4q6kiPLGnnmQjd3xubic4OIldd 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 iQEcBAEBCAAGBQJV8wjXAAoJEDuxQgLoOKytp40H/iqnnPrCecj8WLLrrdcXzek+ c2GXIFm2AdmRqrZfTyYJ4Z/Core1oQJIeMqrDNSCRhX/JSsPJDCK4XeN262YysAM cCDP7lvj4pVcHxnx8R85vpyN3aN8DivW5jo9UoKULx1Um5sVsIisMmzn3aRU1Vj+ snQKfTK0osxXJUwJEo+hGEaq6x6u1YFtlYFdzQXpxDVn6J4xc7s6aJo9e6/sgbBB l/yad+xBKOAoGURfNqcU2SxbR+dtS5qFY7jEAtGFkviAmn4If5OI1vfMX6X646ns L09jyR+62hafhyW05eJJp+QgrHjMKlotvrzsZPU6PgE3LatCrrTIaP0z1y9tvLo= =yxO3 -----END PGP SIGNATURE----- --jGbbKak4q6kiPLGnnmQjd3xubic4OIldd--