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

[Kevin Wolf <kwolf@redhat.com>]

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 <vsementsov@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