Re: [Qemu-devel] [PATCH 1/2] blockdev: Fix regression with the default naming of throttling groups

[Max Reitz <mreitz@redhat.com>]

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

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 <berto@igalia.com>
> Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: qemu-stable@nongnu.org
> ---
>  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 <mreitz@redhat.com>

>      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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]