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

[Max Reitz]

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. 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?

Max

>> +    if (!bs) {
>> +        error_setg(errp, "Node '%s' not found", node_name);
>> +        return;
>> +    }
>> +
>> +    if (bs->blk) {
>> +        error_setg(errp, "Node '%s' is already in use by '%s'", node_name,
>> +                   blk_name(bs->blk));
>> +        return;
>> +    }
>> +
>> +    qmp_blockdev_insert_anon_medium(device, bs, errp);
>> +}
> 
> Kevin
>

signature.asc
Description: OpenPGP digital signature