Re: [Qemu-devel] [PATCH v7 31/39] blockdev: Add blockdev-insert-medium

[Kevin Wolf]

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

pgpZKFuQVpjfe.pgp
Description: PGP signature