[Qemu-devel] [PATCH] block: make 'stats-intervals' a list of integers

[Qemu-devel] [PATCH] block: make 'stats-intervals' a list of integers

[Alberto Garcia]

This is the natural JSON representation and prevents us from having to
decode the list manually.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c             | 64 ++++++++++++++++++++++++++++++++++----------------
 qapi/block-core.json   |  7 +++---
 tests/qemu-iotests/136 |  2 +-
 3 files changed, 48 insertions(+), 25 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index fc85128..a35a559 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -442,13 +442,14 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
     int bdrv_flags = 0;
     int on_read_error, on_write_error;
     bool account_invalid, account_failed;
-    const char *stats_intervals;
     BlockBackend *blk;
     BlockDriverState *bs;
     ThrottleConfig cfg;
     int snapshot = 0;
     Error *error = NULL;
     QemuOpts *opts;
+    QDict *interval_dict;
+    QList *interval_list;
     const char *id;
     bool has_driver_specific_opts;
     BlockdevDetectZeroesOptions detect_zeroes =
@@ -482,7 +483,14 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
     account_invalid = qemu_opt_get_bool(opts, "stats-account-invalid", true);
     account_failed = qemu_opt_get_bool(opts, "stats-account-failed", true);
 
-    stats_intervals = qemu_opt_get(opts, "stats-intervals");
+    qdict_extract_subqdict(bs_opts, &interval_dict, "stats-intervals.");
+    qdict_array_split(interval_dict, &interval_list);
+
+    if (qdict_size(interval_dict) != 0) {
+        error_setg(errp, "Invalid option stats-intervals.%s",
+                   qdict_first(interval_dict)->key);
+        goto early_err;
+    }
 
     extract_common_blockdev_options(opts, &bdrv_flags, &throttling_group, &cfg,
                                     &detect_zeroes, &error);
@@ -555,6 +563,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
 
         QDECREF(bs_opts);
     } else {
+        const QListEntry *entry;
         if (file && !*file) {
             file = NULL;
         }
@@ -583,32 +592,48 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
 
         block_acct_init(blk_get_stats(blk), account_invalid, account_failed);
 
-        if (stats_intervals) {
-            char **intervals = g_strsplit(stats_intervals, ":", 0);
-            unsigned i;
+        for (entry = qlist_first(interval_list); entry;
+             entry = qlist_next(entry)) {
+            unsigned length;
+            switch (qobject_type(entry->value)) {
 
-            if (*stats_intervals == '\0') {
-                error_setg(&error, "stats-intervals can't have an empty value");
-            }
-
-            for (i = 0; !error && intervals[i] != NULL; i++) {
+            case QTYPE_QSTRING: {
                 unsigned long long val;
-                if (parse_uint_full(intervals[i], &val, 10) == 0 &&
+                const char *str;
+                str = qstring_get_str(qobject_to_qstring(entry->value));
+                if (parse_uint_full(str, &val, 10) == 0 &&
                     val > 0 && val <= UINT_MAX) {
-                    block_acct_add_interval(blk_get_stats(blk), val);
+                    length = val;
                 } else {
-                    error_setg(&error, "Invalid interval length: '%s'",
-                               intervals[i]);
+                    error_setg(&error, "Invalid interval length: %s", str);
                 }
+                break;
             }
 
-            g_strfreev(intervals);
+            case QTYPE_QINT: {
+                int64_t val;
+                val = qint_get_int(qobject_to_qint(entry->value));
+                if (val > 0 && val <= UINT_MAX) {
+                    length = val;
+                } else {
+                    error_setg(&error, "Invalid interval length: %" PRId64,
+                               val);
+                }
+                break;
+            }
+
+            default:
+                error_setg(&error,
+                           "The specification of stats-intervals is invalid");
+            }
 
             if (error) {
                 error_propagate(errp, error);
                 blk_unref(blk);
                 blk = NULL;
                 goto err_no_bs_opts;
+            } else {
+                block_acct_add_interval(blk_get_stats(blk), length);
             }
         }
     }
@@ -617,10 +642,14 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
 
 err_no_bs_opts:
     qemu_opts_del(opts);
+    QDECREF(interval_dict);
+    QDECREF(interval_list);
     return blk;
 
 early_err:
     qemu_opts_del(opts);
+    QDECREF(interval_dict);
+    QDECREF(interval_list);
 err_no_opts:
     QDECREF(bs_opts);
     return NULL;
@@ -3948,11 +3977,6 @@ QemuOptsList qemu_common_drive_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "whether to account for failed I/O operations "
                     "in the statistics",
-        },{
-            .name = "stats-intervals",
-            .type = QEMU_OPT_STRING,
-            .help = "colon-separated list of intervals "
-                    "for collecting I/O statistics, in seconds",
         },
         { /* end of list */ }
     },
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f97c250..a07b13f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1531,9 +1531,8 @@
 # @stats-account-failed: #optional whether to include failed
 #                         operations when computing latency and last
 #                         access statistics (default: true) (Since 2.5)
-# @stats-intervals: #optional colon-separated list of intervals for
-#                   collecting I/O statistics, in seconds (default: none)
-#                   (Since 2.5)
+# @stats-intervals: #optional list of intervals for collecting I/O
+#                   statistics, in seconds (default: none) (Since 2.5)
 # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1)
 #                 (default: off)
 #
@@ -1551,7 +1550,7 @@
             '*read-only': 'bool',
             '*stats-account-invalid': 'bool',
             '*stats-account-failed': 'bool',
-            '*stats-intervals': 'str',
+            '*stats-intervals': ['int'],
             '*detect-zeroes': 'BlockdevDetectZeroesOptions' } }
 
 ##
diff --git a/tests/qemu-iotests/136 b/tests/qemu-iotests/136
index f574d83..e8c6937 100644
--- a/tests/qemu-iotests/136
+++ b/tests/qemu-iotests/136
@@ -69,7 +69,7 @@ sector = "%d"
 
     def setUp(self):
         drive_args = []
-        drive_args.append("stats-intervals=%d" % interval_length)
+        drive_args.append("stats-intervals.0=%d" % interval_length)
         drive_args.append("stats-account-invalid=%s" %
                           (self.account_invalid and "on" or "off"))
         drive_args.append("stats-account-failed=%s" %
-- 
2.6.2

Re: [Qemu-devel] [PATCH for-2.5] block: make 'stats-intervals' a list of integers

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]

On 11/11/2015 06:17 AM, Alberto Garcia wrote:
> This is the natural JSON representation and prevents us from having to
> decode the list manually.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c             | 64 ++++++++++++++++++++++++++++++++++----------------
>  qapi/block-core.json   |  7 +++---
>  tests/qemu-iotests/136 |  2 +-
>  3 files changed, 48 insertions(+), 25 deletions(-)
> 

> +        for (entry = qlist_first(interval_list); entry;
> +             entry = qlist_next(entry)) {
> +            unsigned length;
> +            switch (qobject_type(entry->value)) {
>  
> -            if (*stats_intervals == '\0') {
> -                error_setg(&error, "stats-intervals can't have an empty value");
> -            }
> -
> -            for (i = 0; !error && intervals[i] != NULL; i++) {
> +            case QTYPE_QSTRING: {

> +            case QTYPE_QINT: {

Why are we accepting both string and int here, but typing it as 'int' in
qapi?  I guess its due to how command line parsing passes in strings
rather than ints?

But that should be okay.

Reviewed-by: Eric Blake <eblake@redhat.com>

We may eventually want to add an alternate type that takes both int and
string from QMP (parsing the port number of server addresses is another
spot that could benefit from such an alternate), but that's a project
for 2.6.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.5] block: make 'stats-intervals' a list of integers

[Alberto Garcia]

On Wed 11 Nov 2015 04:32:42 PM CET, Eric Blake wrote:
>> -            for (i = 0; !error && intervals[i] != NULL; i++) {
>> +            case QTYPE_QSTRING: {
>
>> +            case QTYPE_QINT: {
>
> Why are we accepting both string and int here, but typing it as 'int' in
> qapi?  I guess its due to how command line parsing passes in strings
> rather than ints?

Yes.

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH] block: make 'stats-intervals' a list of integers

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 830 bytes --]

On Wed, Nov 11, 2015 at 03:17:12PM +0200, Alberto Garcia wrote:
> This is the natural JSON representation and prevents us from having to
> decode the list manually.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c             | 64 ++++++++++++++++++++++++++++++++++----------------
>  qapi/block-core.json   |  7 +++---
>  tests/qemu-iotests/136 |  2 +-
>  3 files changed, 48 insertions(+), 25 deletions(-)

gcc 5.1.1 warning:

blockdev.c: In function ‘blockdev_init’:
blockdev.c:636:17: error: ‘length’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
                 block_acct_add_interval(blk_get_stats(blk), length);
                 ^
blockdev.c:597:22: note: ‘length’ was declared here
             unsigned length;
                      ^

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] block: make 'stats-intervals' a list of integers

[Alberto Garcia]

On Fri 13 Nov 2015 11:15:52 AM CET, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> blockdev.c: In function ‘blockdev_init’:
> blockdev.c:636:17: error: ‘length’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>                  block_acct_add_interval(blk_get_stats(blk), length);
>                  ^
> blockdev.c:597:22: note: ‘length’ was declared here
>              unsigned length;
>                       ^

That's a false warning because length can only be uninitialized if
'error' is set.

gcc 5.2.1 does not complain here... anyway, this should fix it:

         for (entry = qlist_first(interval_list); entry;
              entry = qlist_next(entry)) {
-            unsigned length;
+            unsigned length = 0;
             switch (qobject_type(entry->value)) {

Do you want me to send a new patch, or do you prefer to apply this
change in your branch? Thanks!

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH] block: make 'stats-intervals' a list of integers

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1318 bytes --]

On Wed, Nov 11, 2015 at 03:17:12PM +0200, Alberto Garcia wrote:
> @@ -583,32 +592,48 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
>  
>          block_acct_init(blk_get_stats(blk), account_invalid, account_failed);
>  
> -        if (stats_intervals) {
> -            char **intervals = g_strsplit(stats_intervals, ":", 0);
> -            unsigned i;
> +        for (entry = qlist_first(interval_list); entry;
> +             entry = qlist_next(entry)) {

This loop could be extracted into a separate function to avoid growing blockdev_init() further:

bool parse_stats_intervals(BlockAcctStats *stats, QList *intervals, Error **errp);

> @@ -617,10 +642,14 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
>  
>  err_no_bs_opts:
>      qemu_opts_del(opts);
> +    QDECREF(interval_dict);
> +    QDECREF(interval_list);
>      return blk;
>  
>  early_err:
>      qemu_opts_del(opts);
> +    QDECREF(interval_dict);
> +    QDECREF(interval_list);

There is a codepath that reaches here without initializing interval_dict
or interval_list:

  qemu_opts_absorb_qdict(opts, bs_opts, &error);
  if (error) {
      error_propagate(errp, error);
      goto early_err;
  }

interval_dict and interval_list should be initialized to NULL.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] block: make 'stats-intervals' a list of integers

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1152 bytes --]

On Fri, Nov 13, 2015 at 11:50:03AM +0100, Alberto Garcia wrote:
> On Fri 13 Nov 2015 11:15:52 AM CET, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > blockdev.c: In function ‘blockdev_init’:
> > blockdev.c:636:17: error: ‘length’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >                  block_acct_add_interval(blk_get_stats(blk), length);
> >                  ^
> > blockdev.c:597:22: note: ‘length’ was declared here
> >              unsigned length;
> >                       ^
> 
> That's a false warning because length can only be uninitialized if
> 'error' is set.
> 
> gcc 5.2.1 does not complain here... anyway, this should fix it:
> 
>          for (entry = qlist_first(interval_list); entry;
>               entry = qlist_next(entry)) {
> -            unsigned length;
> +            unsigned length = 0;
>              switch (qobject_type(entry->value)) {
> 
> Do you want me to send a new patch, or do you prefer to apply this
> change in your branch? Thanks!

I just noticed another issue that will require another revision (unless
I misread the code).

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] block: make 'stats-intervals' a list of integers

[Alberto Garcia]

On Mon 16 Nov 2015 04:46:44 AM CET, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>  early_err:
>>      qemu_opts_del(opts);
>> +    QDECREF(interval_dict);
>> +    QDECREF(interval_list);
>
> There is a codepath that reaches here without initializing interval_dict
> or interval_list:
>
>   qemu_opts_absorb_qdict(opts, bs_opts, &error);
>   if (error) {
>       error_propagate(errp, error);
>       goto early_err;
>   }

You're right, I'll send a new version ASAP.

Berto