From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41937) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZpdiI-0005YF-An for qemu-devel@nongnu.org; Fri, 23 Oct 2015 10:53:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZpdiH-0003s0-4p for qemu-devel@nongnu.org; Fri, 23 Oct 2015 10:53:02 -0400 Date: Fri, 23 Oct 2015 16:52:48 +0200 From: Kevin Wolf Message-ID: <20151023145248.GM3797@noname.redhat.com> References: <1445270025-22999-1-git-send-email-mreitz@redhat.com> <1445270025-22999-32-git-send-email-mreitz@redhat.com> <20151023134219.GE3797@noname.redhat.com> <562A45B1.60107@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="vEao7xgI/oilGqZ+" Content-Disposition: inline In-Reply-To: <562A45B1.60107@redhat.com> Subject: Re: [Qemu-devel] [PATCH v7 31/39] blockdev: Add blockdev-insert-medium List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: Alberto Garcia , qemu-block@nongnu.org, John Snow , qemu-devel@nongnu.org, Markus Armbruster , Stefan Hajnoczi --vEao7xgI/oilGqZ+ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Am 23.10.2015 um 16:35 hat Max Reitz geschrieben: > On 23.10.2015 15:42, Kevin Wolf wrote: > > Am 19.10.2015 um 17:53 hat Max Reitz geschrieben: > >> 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 | 54 +++++++++++++++++++++++++++++++++++++++++++= +++++++++ > >> qapi/block-core.json | 17 +++++++++++++++++ > >> qmp-commands.hx | 37 +++++++++++++++++++++++++++++++++++ > >> 3 files changed, 108 insertions(+) > >> > >> diff --git a/blockdev.c b/blockdev.c > >> index a8601ca..a4c278f 100644 > >> --- a/blockdev.c > >> +++ b/blockdev.c > >> @@ -2151,6 +2151,60 @@ void qmp_blockdev_remove_medium(const char *dev= ice, Error **errp) > >> } > >> } > >> =20 > >> +static void qmp_blockdev_insert_anon_medium(const char *device, > >> + BlockDriverState *bs, Err= or **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", 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 =3D bdrv_find_node(node_name); > >=20 > > Shouldn't this use bdrv_lookup_bs() to accept a BB name and be consisent > > with most other commands? Of course, if you actually used a BB name, it > > would fail below, but not with a confusing "not found" message. >=20 > Well, and then it fails with "Node 'foo' is already in use by 'foo'", > which doesn't make much more sense either. >=20 > Until we support multiple BBs per BDS, using this command with a BB > doesn't make any sense. Correct, this would be mostly in preparation for supporting multiple BBs per BDS. > I may be wrong here or exaggerating, but I feel > like most of the "most other commands" allow that mostly because of > legacy reasons. Second, most of them are block jobs which I feel like > should work behind a BB anyway, and letting them work on a BDS is wrong, > but we just haven't converted them yet. >=20 > I don't have a strong preference. I find the error messages equally bad. > But I think I don't want to use bdrv_lookup_bs() since that would look > like pretending that we actually do want to support BB names, whereas in > reality we absolutely don't (not right now at least). >=20 > Also, it would confuse me when reading the code: "Why are you accepting > a BB name up there, and then you are rejecting every BDS that has a BB? > That doesn't make sense!" >=20 > Improving the error message doesn't seem to good to me either. It would > have to be something like "'%s' is a device, not a node" which I don't > consider much more helpful either (maybe it is, I don't know), and > adding an explanation like "; blockdev-insert-medium only accepts node > names" would feel like a bit too much since we don't do that anywhere > else, do we? Fair enough. It's your code, you decide. Kevin --vEao7xgI/oilGqZ+ Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJWKknAAAoJEH8JsnLIjy/WJDQP/RkywXxJHy7Zu1aDvTc2+z7k LIO8oyTCubhJ/N3UNj3U0RILK0LEgt8TJQ1Wed1w8420iDF0J5+DE2qSlTbFT1e/ tSWy8eb/KJREz/WmvhygN/ny/UqdTt9voYDX9lqgZKXAe5RovIzNOEuQho4UkdKC lYV5jRUfzgiUa+UMoSpTmO/ibMJZxZK5loI1zl5RtxGNUtdfIcpvx3grWmC2tthv SOjRYr1BQIus+9TCnadDrYLBJh2/v+J7e647bx9h7f04/hSMyUlvgEcDsc7YMFfK qvXEZgmgbJj4srVB4iCoZeF39uiZ3uNSl61olZgKPSGN+idctDh8s2HQ40RNZ2N6 VwnFiMPNghuXR0QyhR1JNGqRv41w/mAVoVK7yEgieg59agYJbZ2FUZPD2PRT5tvI U/VU9uRzhjCaN7zm/Iy+kfiykWlmsmEqgv8QFwafp2S4lXLTQOpMbHYJjPnWTLLy LIGB7tzTtGf8MPxC8wCDUE6hxdOKXxiS75rdPZAKsfqu6M+4FnZqpuPRmXCdKGH5 HW5hb18JeMoXmrJIF1SMoUW5R7JYWDEHv3ox6N8P+KfoEV9OgLfFe2LYUv/OexOw Azcj+TX4XP4buhlFTb/gpwT1DrdtfSmK1NktI5cuEeY/EI2sJFh4FZuGuO38n0z9 mW499OQM84P1oemj8C0e =R1/+ -----END PGP SIGNATURE----- --vEao7xgI/oilGqZ+--