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