qemu-stable
[Top][All Lists]
Advanced

[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;
> 


Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]