[Qemu-devel] [PATCH v3 0/2] improve block-latency-histogram-set

[Qemu-devel] [PATCH v3 0/2] improve block-latency-histogram-set

[Vladimir Sementsov-Ogievskiy]

Support QOM path for block-latency-histogram-set and drop x- prefix.

Libvirt discussion (not yet updated for using id instead of device):
https://www.redhat.com/archives/libvir-list/2018-September/msg00011.html

v3: rebase on master, s/3.1/4.0.

v2: id,device -> id (with fall back to device name)
    fix versions and all remaining x_ prefixes

Vladimir Sementsov-Ogievskiy (2):
  qapi: move to QOM path for x-block-latency-histogram-set
  qapi: drop x- from x-block-latency-histogram-set

 qapi/block-core.json | 24 ++++++++++++------------
 block/qapi.c         | 12 ++++++------
 blockdev.c           | 24 ++++++++++++++++--------
 3 files changed, 34 insertions(+), 26 deletions(-)

-- 
2.18.0

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

[Vladimir Sementsov-Ogievskiy]

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;
+        }
     }
+
     stats = blk_get_stats(blk);
 
     if (!has_boundaries && !has_boundaries_read && !has_boundaries_write &&
@@ -4426,7 +4434,7 @@ void qmp_x_block_latency_histogram_set(
             stats, BLOCK_ACCT_READ,
             has_boundaries_read ? boundaries_read : boundaries);
         if (ret) {
-            error_setg(errp, "Device '%s' set read boundaries fail", device);
+            error_setg(errp, "Device '%s' set read boundaries fail", id);
             return;
         }
     }
@@ -4436,7 +4444,7 @@ void qmp_x_block_latency_histogram_set(
             stats, BLOCK_ACCT_WRITE,
             has_boundaries_write ? boundaries_write : boundaries);
         if (ret) {
-            error_setg(errp, "Device '%s' set write boundaries fail", device);
+            error_setg(errp, "Device '%s' set write boundaries fail", id);
             return;
         }
     }
@@ -4446,7 +4454,7 @@ void qmp_x_block_latency_histogram_set(
             stats, BLOCK_ACCT_FLUSH,
             has_boundaries_flush ? boundaries_flush : boundaries);
         if (ret) {
-            error_setg(errp, "Device '%s' set flush boundaries fail", device);
+            error_setg(errp, "Device '%s' set flush boundaries fail", id);
             return;
         }
     }
-- 
2.18.0

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

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 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

Is such overloaded semantics what we want in new interfaces?

Hmm, looks like there's ample precedence for it.  Escaped my grep at
first because its commonly documented as "The name or QOM path of the
guest device".  Suggest to stick to that for consistency.

> @@ -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'],
[...]

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

[Vladimir Sementsov-Ogievskiy]

08.01.2019 16:20, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> 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
> 
> Is such overloaded semantics what we want in new interfaces?
> 
> Hmm, looks like there's ample precedence for it.  Escaped my grep at
> first because its commonly documented as "The name or QOM path of the
> guest device".  Suggest to stick to that for consistency.


Interesting, that in cases you mean, documentation seems wrong:
it goes through qmp_get_blk, which works like @id may be only QOM path, not name,
so for the it should be @id: The QOM path.

And I'm afraid that only my case merges both into one field. And it is more correct
to put QOM path in first place, as it is checked first, and if it fails, searches by
name. So, I think we should keep it as is, and fix other command documentation (but
not in these series, of course)

> 
>> @@ -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'],
> [...]
> 


-- 
Best regards,
Vladimir

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

[Kevin Wolf]

Am 11.02.2019 um 18:39 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 08.01.2019 16:20, Markus Armbruster wrote:
> > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> > 
> >> 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
> > 
> > Is such overloaded semantics what we want in new interfaces?
> > 
> > Hmm, looks like there's ample precedence for it.  Escaped my grep at
> > first because its commonly documented as "The name or QOM path of the
> > guest device".  Suggest to stick to that for consistency.
> 
> 
> Interesting, that in cases you mean, documentation seems wrong:
> it goes through qmp_get_blk, which works like @id may be only QOM path, not name,
> so for the it should be @id: The QOM path.

It's really a QOM path relative to /machine/peripheral (see
find_device_state()), which is where named devices live, accessible
through their id. So relative paths are both QOM paths and names of
guest devices. (Relative paths aren't a QOM concept, though, which
provides only absolute and partial paths. The relative paths have a
one-off implementation here.)

So in the end, I think the description is actually correct, just with a
higher level perspective, ignoring all the low-level details.

Kevin

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

[Vladimir Sementsov-Ogievskiy]

11.02.2019 20:52, Kevin Wolf wrote:
> Am 11.02.2019 um 18:39 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 08.01.2019 16:20, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>
>>>> 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
>>>
>>> Is such overloaded semantics what we want in new interfaces?
>>>
>>> Hmm, looks like there's ample precedence for it.  Escaped my grep at
>>> first because its commonly documented as "The name or QOM path of the
>>> guest device".  Suggest to stick to that for consistency.
>>
>>
>> Interesting, that in cases you mean, documentation seems wrong:
>> it goes through qmp_get_blk, which works like @id may be only QOM path, not name,
>> so for the it should be @id: The QOM path.
> 
> It's really a QOM path relative to /machine/peripheral (see
> find_device_state()), which is where named devices live, accessible
> through their id. So relative paths are both QOM paths and names of
> guest devices. (Relative paths aren't a QOM concept, though, which
> provides only absolute and partial paths. The relative paths have a
> one-off implementation here.)
> 
> So in the end, I think the description is actually correct, just with a
> higher level perspective, ignoring all the low-level details.
> 

Hmm I thought, that name is an argument of blk_by_name() and path is an
argument of blk_by_qdev_id.. But you say that argument of blk_by_qdev_id
may be considered as "name" too, at least for user. If so, that is ok..

Consider more context:

   # @device: Block device name (deprecated, use @id instead)
   #
   # @id:     The name or QOM path of the guest device (since: 2.8)


so, "name" in first line and "name" in second are different kinds of name?

-- 
Best regards,
Vladimir

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

[Kevin Wolf]

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?

Kevin

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

[Vladimir Sementsov-Ogievskiy]

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.

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?


-- 
Best regards,
Vladimir

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

[Kevin Wolf]

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

Re: [Qemu-devel] [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 <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.
> 

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


-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH v3 2/2] qapi: drop x- from x-block-latency-histogram-set

[Vladimir Sementsov-Ogievskiy]

Drop x- and x_ prefixes for latency histograms and update version to
3.1

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json | 20 ++++++++++----------
 block/qapi.c         | 12 ++++++------
 blockdev.c           |  2 +-
 3 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index bb70c51a57..c3fa2e304b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -476,13 +476,13 @@
 #         +------------------
 #             10   50   100
 #
-# Since: 2.12
+# Since: 4.0
 ##
 { 'struct': 'BlockLatencyHistogramInfo',
   'data': {'boundaries': ['uint64'], 'bins': ['uint64'] } }
 
 ##
-# @x-block-latency-histogram-set:
+# @block-latency-histogram-set:
 #
 # Manage read, write and flush latency histograms for the device.
 #
@@ -512,7 +512,7 @@
 #
 # Returns: error if device is not found or any boundary arrays are invalid.
 #
-# Since: 2.12
+# Since: 4.0
 #
 # Example: set new histograms for all io types with intervals
 # [0, 10), [10, 50), [50, 100), [100, +inf):
@@ -546,7 +546,7 @@
 #      "arguments": { "device": "drive0" } }
 # <- { "return": {} }
 ##
-{ 'command': 'x-block-latency-histogram-set',
+{ 'command': 'block-latency-histogram-set',
   'data': {'id': 'str',
            '*boundaries': ['uint64'],
            '*boundaries-read': ['uint64'],
@@ -833,11 +833,11 @@
 # @timed_stats: Statistics specific to the set of previously defined
 #               intervals of time (Since 2.5)
 #
-# @x_rd_latency_histogram: @BlockLatencyHistogramInfo. (Since 2.12)
+# @rd_latency_histogram: @BlockLatencyHistogramInfo. (Since 4.0)
 #
-# @x_wr_latency_histogram: @BlockLatencyHistogramInfo. (Since 2.12)
+# @wr_latency_histogram: @BlockLatencyHistogramInfo. (Since 4.0)
 #
-# @x_flush_latency_histogram: @BlockLatencyHistogramInfo. (Since 2.12)
+# @flush_latency_histogram: @BlockLatencyHistogramInfo. (Since 4.0)
 #
 # Since: 0.14.0
 ##
@@ -852,9 +852,9 @@
            'invalid_wr_operations': 'int', 'invalid_flush_operations': 'int',
            'account_invalid': 'bool', 'account_failed': 'bool',
            'timed_stats': ['BlockDeviceTimedStats'],
-           '*x_rd_latency_histogram': 'BlockLatencyHistogramInfo',
-           '*x_wr_latency_histogram': 'BlockLatencyHistogramInfo',
-           '*x_flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
+           '*rd_latency_histogram': 'BlockLatencyHistogramInfo',
+           '*wr_latency_histogram': 'BlockLatencyHistogramInfo',
+           '*flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
 
 ##
 # @BlockStats:
diff --git a/block/qapi.c b/block/qapi.c
index c66f949db8..dc7508a06d 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -492,14 +492,14 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk)
     }
 
     bdrv_latency_histogram_stats(&stats->latency_histogram[BLOCK_ACCT_READ],
-                                 &ds->has_x_rd_latency_histogram,
-                                 &ds->x_rd_latency_histogram);
+                                 &ds->has_rd_latency_histogram,
+                                 &ds->rd_latency_histogram);
     bdrv_latency_histogram_stats(&stats->latency_histogram[BLOCK_ACCT_WRITE],
-                                 &ds->has_x_wr_latency_histogram,
-                                 &ds->x_wr_latency_histogram);
+                                 &ds->has_wr_latency_histogram,
+                                 &ds->wr_latency_histogram);
     bdrv_latency_histogram_stats(&stats->latency_histogram[BLOCK_ACCT_FLUSH],
-                                 &ds->has_x_flush_latency_histogram,
-                                 &ds->x_flush_latency_histogram);
+                                 &ds->has_flush_latency_histogram,
+                                 &ds->flush_latency_histogram);
 }
 
 static BlockStats *bdrv_query_bds_stats(BlockDriverState *bs,
diff --git a/blockdev.c b/blockdev.c
index ff0d8ded5e..cb95a4b607 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4396,7 +4396,7 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread,
     aio_context_release(old_context);
 }
 
-void qmp_x_block_latency_histogram_set(
+void qmp_block_latency_histogram_set(
     const char *id,
     bool has_boundaries, uint64List *boundaries,
     bool has_boundaries_read, uint64List *boundaries_read,
-- 
2.18.0