| Message ID | af5cd58bd2c4b9f6c57f260d9cfe586b9fb7d34d.1467986342.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; >
On Fri 08 Jul 2016 04:23:03 PM CEST, Max Reitz wrote: >> + 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> Ok, if this series doesn't get a new version feel free to modify the patch when you apply it. Berto
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); + 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;
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(-)