Re: [Qemu-block] [PATCH v3 1/2] qapi: move to QOM path for x-block-latency-histogram-set

[Vladimir Sementsov-Ogievskiy]

12.02.2019 12:07, Kevin Wolf wrote:
> Am 11.02.2019 um 19:30 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 11.02.2019 20:54, Kevin Wolf wrote:
>>> Am 21.12.2018 um 17:53 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> Move to way of device selecting, however fall back to device name if
>>>> path is not found.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>>> ---
>>>>    qapi/block-core.json |  4 ++--
>>>>    blockdev.c           | 22 +++++++++++++++-------
>>>>    2 files changed, 17 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index 762000f31f..bb70c51a57 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -489,7 +489,7 @@
>>>>    # If only @device parameter is specified, remove all present latency 
>>>> histograms
>>>>    # for the device. Otherwise, add/reset some of (or all) latency 
>>>> histograms.
>>>>    #
>>>> -# @device: device name to set latency histogram for.
>>>> +# @id: The QOM path or name of the guest device.
>>>>    #
>>>>    # @boundaries: list of interval boundary values (see description in
>>>>    #              BlockLatencyHistogramInfo definition). If specified, all
>>>> @@ -547,7 +547,7 @@
>>>>    # <- { "return": {} }
>>>>    ##
>>>>    { 'command': 'x-block-latency-histogram-set',
>>>> -  'data': {'device': 'str',
>>>> +  'data': {'id': 'str',
>>>>               '*boundaries': ['uint64'],
>>>>               '*boundaries-read': ['uint64'],
>>>>               '*boundaries-write': ['uint64'],
>>>> diff --git a/blockdev.c b/blockdev.c
>>>> index a6f71f9d83..ff0d8ded5e 100644
>>>> --- a/blockdev.c
>>>> +++ b/blockdev.c
>>>> @@ -4397,21 +4397,29 @@ void qmp_x_blockdev_set_iothread(const char 
>>>> *node_name, StrOrNull *iothread,
>>>>    }
>>>>    
>>>>    void qmp_x_block_latency_histogram_set(
>>>> -    const char *device,
>>>> +    const char *id,
>>>>        bool has_boundaries, uint64List *boundaries,
>>>>        bool has_boundaries_read, uint64List *boundaries_read,
>>>>        bool has_boundaries_write, uint64List *boundaries_write,
>>>>        bool has_boundaries_flush, uint64List *boundaries_flush,
>>>>        Error **errp)
>>>>    {
>>>> -    BlockBackend *blk = blk_by_name(device);
>>>>        BlockAcctStats *stats;
>>>>        int ret;
>>>> +    Error *local_err = NULL;
>>>> +    BlockBackend *blk = blk_by_qdev_id(id, &local_err);
>>>>    
>>>>        if (!blk) {
>>>> -        error_setg(errp, "Device '%s' not found", device);
>>>> -        return;
>>>> +        blk = blk_by_name(id);
>>>> +        if (!blk) {
>>>> +            error_propagate(errp, local_err);
>>>> +            return;
>>>> +        } else {
>>>> +            error_free(local_err);
>>>> +            local_err = NULL;
>>>> +        }
>>>>        }
>>>
>>> Why don't you use qmp_get_blk() here?
>>>
>>
>> qmp_get_blk is used for cases when we have both @device and @id parameters, 
>> where @device is deprecated.
>>
>> And one of previous iteration was a try to do it in this way (have both @id 
>> and @device). But it was
>> considered to be bad idea for a new command, which don't need any backward 
>> compatibility. And we decided
>> to merge two variants in @id.
> 
> Ah, right I missed that you dropped @device and just noticed that this
> looks different from other commands. Calling blk_by_qdev_id() directly
> makes sense then.
> 
>> So, do you suggest to drop blk_by_name() variant at all, and go through 
>> qmp_get_blk(NULL, id), which
>> will call blk_by_qdev_id() and fail if not found?
> 
> Yes, we don't need the blk_by_name() path, it's only for the legacy
> @device options.
> 
> In any case, you can't use the same option for blk_by_name() and
> blk_by_qdev_id(). They are separate namespaces and the name could exist
> in both of them. This is why the other commands have both @id and
> @device instead of just a single option.
> 

Ok, thank you for explanation. I'll resend today.


-- 
Best regards,
Vladimir