[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-stable] [PATCH 1/2] blockdev: Fix regression with the default
From: |
Max Reitz |
Subject: |
Re: [Qemu-stable] [PATCH 1/2] blockdev: Fix regression with the default naming of throttling groups |
Date: |
Fri, 8 Jul 2016 16:23:03 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 |
On 08.07.2016 16:03, Alberto Garcia wrote:
> When I/O limits are set for a block device, the name of the throttling
> group is taken from the BlockBackend if the user doesn't specify one.
>
> Commit efaa7c4eeb7490c6f37f3 moved the naming of the BlockBackend in
> blockdev_init() to the end of the function, after I/O limits are set.
> The consequence is that the throttling group gets an empty name.
>
> Signed-off-by: Alberto Garcia <address@hidden>
> Reported-by: Stefan Hajnoczi <address@hidden>
> Cc: Max Reitz <address@hidden>
> Cc: address@hidden
> ---
> blockdev.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 0f8065c..3ad7d29 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -483,6 +483,7 @@ static BlockBackend *blockdev_init(const char *file,
> QDict *bs_opts,
> const char *id;
> BlockdevDetectZeroesOptions detect_zeroes =
> BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
> + const char *blk_id;
> const char *throttling_group = NULL;
>
> /* Check common options by copying from bs_opts to opts, all other
> options
> @@ -512,6 +513,8 @@ static BlockBackend *blockdev_init(const char *file,
> QDict *bs_opts,
>
> writethrough = !qemu_opt_get_bool(opts, BDRV_OPT_CACHE_WB, true);
>
> + blk_id = qemu_opts_id(opts);
> +
Side note: The "id" variable is supposed to contain the exact same
value, but the string it points to is invalidated by the
qdict_del(bs_opts, "id") call.
So indeed we need to obtain the ID anew here (or we'd have to do
g_strdup() before and g_free() after, which is cumbersome). But
regarding the variable itself, you could have actually reused "id" (I
only noticed that just now).
But that's just a minor thing, so:
Reviewed-by: Max Reitz <address@hidden>
> qdict_extract_subqdict(bs_opts, &interval_dict, "stats-intervals.");
> qdict_array_split(interval_dict, &interval_list);
>
> @@ -616,7 +619,7 @@ static BlockBackend *blockdev_init(const char *file,
> QDict *bs_opts,
> /* disk I/O throttling */
> if (throttle_enabled(&cfg)) {
> if (!throttling_group) {
> - throttling_group = blk_name(blk);
> + throttling_group = blk_id;
> }
> blk_io_limits_enable(blk, throttling_group);
> blk_set_io_limits(blk, &cfg);
> @@ -625,7 +628,7 @@ static BlockBackend *blockdev_init(const char *file,
> QDict *bs_opts,
> blk_set_enable_write_cache(blk, !writethrough);
> blk_set_on_error(blk, on_read_error, on_write_error);
>
> - if (!monitor_add_blk(blk, qemu_opts_id(opts), errp)) {
> + if (!monitor_add_blk(blk, blk_id, errp)) {
> blk_unref(blk);
> blk = NULL;
> goto err_no_bs_opts;
>
signature.asc
Description: OpenPGP digital signature