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 <vsement...@virtuozzo.com> >>>> --- >>>> 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