[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v7 31/39] blockdev: Add blockdev-insert-medium
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-block] [PATCH v7 31/39] blockdev: Add blockdev-insert-medium |
Date: |
Fri, 23 Oct 2015 16:52:48 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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 <address@hidden>
> >> ---
> >> 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 *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);
> >
> > 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.
>
> Well, and then it fails with "Node 'foo' is already in use by 'foo'",
> which doesn't make much more sense either.
>
> 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.
>
> 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).
>
> 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!"
>
> 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
pgpsxnHdeBrHn.pgp
Description: PGP signature
[Qemu-block] [PATCH v7 32/39] blockdev: Implement eject with basic operations, Max Reitz, 2015/10/19
[Qemu-block] [PATCH v7 33/39] blockdev: Implement change with basic operations, Max Reitz, 2015/10/19
[Qemu-block] [PATCH v7 34/39] block: Inquire tray state before tray-moved events, Max Reitz, 2015/10/19
[Qemu-block] [PATCH v7 35/39] qmp: Introduce blockdev-change-medium, Max Reitz, 2015/10/19