Message ID | 1447247832-14198-1-git-send-email-berto@igalia.com |
---|---|
State | New |
Headers | show |
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.
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
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; ^
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
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.
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
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
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" %
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(-)