block: make 'stats-interval' an array of ints instead of a string
v2 cover block: make 'stats-interval' an array of ints instead of a string
Second version of the patch, following all suggestions by Stefan.
v2:
- The code that parses the intervals has been moved to a separate function
- Due to the previous change, the loop has been modified a bit and the original 'length' variable no longer exists.
- interval_dict and interval_list are now initialized to NULL
v1: https://lists.gnu.org/archive/html/qemu-block/2015-11/msg00432.html
Regards,
Berto
Alberto Garcia (1):
block: make 'stats-interval' an array of ints instead of a string
blockdev.c | 91 +++++++++++++++++++++++++++++++------------------- qapi/block-core.json | 7 ++-- tests/qemu-iotests/136 | 2 +- 3 files changed, 61 insertions(+), 39 deletions(-)
Signature · 1 line
2.6.2
PATCH v2 1/1 block: make 'stats-interval' an array of ints instead of a string
This is the natural JSON representation and prevents us from having to decode the list manually.
--- blockdev.c | 91 +++++++++++++++++++++++++++++++------------------- qapi/block-core.json | 7 ++-- tests/qemu-iotests/136 | 2 +- 3 files changed, 61 insertions(+), 39 deletions(-)
blockdev.c +57 -34
@@ -300,6 +300,45 @@ static int parse_block_error_action(const char *buf, bool is_read, Error **errp) } } +static bool parse_stats_intervals (BlockAcctStats *stats,QList *intervals, +Error **errp) +{ +const QListEntry *entry; +for (entry =qlist_first (intervals); entry; entry =qlist_next (entry)) { +switch (qobject_type (entry->value)) { + +case QTYPE_QSTRING : { +unsigned long long length; +const char *str =qstring_get_str (qobject_to_qstring (entry->value)); +if (parse_uint_full (str, &length,10 ) ==0 && + length >0 && length <=UINT_MAX ) { +block_acct_add_interval (stats, (unsigned ) length); + }else { +error_setg (errp,"Invalid interval length: %s" , str); +return false; + } +break ; + } + +case QTYPE_QINT : { +int64_t length =qint_get_int (qobject_to_qint (entry->value)); +if (length >0 && length <=UINT_MAX ) { +block_acct_add_interval (stats, (unsigned ) length); + }else { +error_setg (errp,"Invalid interval length: %" PRId64, length); +return false; + } +break ; + } + +default : +error_setg (errp,"The specification of stats-intervals is invalid" ); +return false; + } + } +return true; +} +static bool check_throttle_config (ThrottleConfig *cfg,Error **errp) {if (throttle_conflicting (cfg)) {
@@ -442,13 +481,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 =NULL ; +QList *interval_list =NULL ;const char *id;bool has_driver_specific_opts; BlockdevDetectZeroesOptions detect_zeroes =
@@ -482,7 +522,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);
@@ -583,33 +630,10 @@ 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; - -if (*stats_intervals =='\0' ) { -error_setg (&error,"stats-intervals can't have an empty value" ); - } - -for (i =0 ; !error && intervals[i] !=NULL ; i++) { -unsigned long long val; -if (parse_uint_full (intervals[i], &val,10 ) ==0 && - val >0 && val <=UINT_MAX ) { -block_acct_add_interval (blk_get_stats (blk), val); - }else { -error_setg (&error,"Invalid interval length: '%s'" , - intervals[i]); - } - } - -g_strfreev (intervals); - -if (error) { -error_propagate (errp, error); -blk_unref (blk); - blk =NULL ; - goto err_no_bs_opts; - } +if (!parse_stats_intervals (blk_get_stats (blk), interval_list, errp)) { +blk_unref (blk); + blk =NULL ; + goto err_no_bs_opts; } }
@@ -617,10 +641,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 +3976,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 */ } },
qapi/block-core.json +3 -4
@@ -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' } } ##
tests/qemu-iotests/136 +1 -1
@@ -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" %
Signature · 1 line
2.6.2
Stefan Hajnoczi
AppliedQuoted 15 lines · Alberto Garcia
Second version of the patch, following all suggestions by Stefan.
v2:
- The code that parses the intervals has been moved to a separate function
- Due to the previous change, the loop has been modified a bit and the original 'length' variable no longer exists.
-interval_dictandinterval_listare now initialized to NULLv1: https://lists.gnu.org/archive/html/qemu-block/2015-11/msg00432.html
Regards,
Berto
Quoted 11 lines
Alberto Garcia (1):
block: make 'stats-interval' an array of ints instead of a stringblockdev.c | 91 +++++++++++++++++++++++++++++++------------------- qapi/block-core.json | 7 ++-- tests/qemu-iotests/136 | 2 +- 3 files changed, 61 insertions(+), 39 deletions(-)Signature · 1 line
2.6.2
Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block
Stefan