[libvirt] [PATCH 0/5] Various blkdeviotune fixes (but mostly group_name, well, almost exclusively)
So I wanted to fix the last thing in Bug 1344897 [1], just two error messages. But I couldn't test it out because recent addition of group_name broke setting blkdeviotune on new enough QEMU. Yes, that means 3.0.0 doesn't work with new enough QEMU. Or namespaces. Still not totally a brown-bag release though. Let's see if we can find more things like that =) [1] https://bugzilla.redhat.com/show_bug.cgi?id=1344897 Martin Kletzander (5): qemu: Only set group_name when actually requested qemu: Don't lose group_name virsh: Actually make blkdeviotune --group_name work virsh: Use consistent naming for blkdeviotune options qemu: Add better message for some invalid block I/O settings src/qemu/qemu_driver.c | 62 ++++++++++++++++++++++++++++++++++---------------- tools/virsh-domain.c | 11 ++++++--- tools/virsh.pod | 4 ++-- 3 files changed, 53 insertions(+), 24 deletions(-) -- 2.11.0
We were setting it based on whether it was supported and that lead to setting it to NULL, which our JSON code caught. However it ended up producing the following results: $ virsh blkdeviotune fedora vda --total-bytes-sec-max 2000 error: Unable to change block I/O throttle error: internal error: argument key 'group' must not have null value Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 516a851d3d55..f45972e3c823 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17506,7 +17506,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, &info, supportMaxOptions, - supportGroupNameOption, + set_fields & QEMU_BLOCK_IOTUNE_SET_GROUP_NAME, supportMaxLengthOptions); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; -- 2.11.0
On 01/25/2017 04:16 AM, Martin Kletzander wrote:
We were setting it based on whether it was supported and that lead to setting it to NULL, which our JSON code caught. However it ended up producing the following results:
$ virsh blkdeviotune fedora vda --total-bytes-sec-max 2000 error: Unable to change block I/O throttle error: internal error: argument key 'group' must not have null value
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 516a851d3d55..f45972e3c823 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17506,7 +17506,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, &info, supportMaxOptions, - supportGroupNameOption, + set_fields & QEMU_BLOCK_IOTUNE_SET_GROUP_NAME, supportMaxLengthOptions); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1;
I believe this should be a change to qemuMonitorJSONSetBlockIoThrottle to use 'S' instead of 's' in the virJSONValueObjectAdd call. Been too long to remember why I went with 's' - although I wonder if it had to do with "unsetting" the value... I do believe I should have gone with my first instincts on this and should have made the design around allowing the user to define a group number to use and then convert that into a group name string with a static prefix and the number <sigh>. ACK with that change. John
On Wed, Jan 25, 2017 at 06:43:39AM -0500, John Ferlan wrote:
On 01/25/2017 04:16 AM, Martin Kletzander wrote:
We were setting it based on whether it was supported and that lead to setting it to NULL, which our JSON code caught. However it ended up producing the following results:
$ virsh blkdeviotune fedora vda --total-bytes-sec-max 2000 error: Unable to change block I/O throttle error: internal error: argument key 'group' must not have null value
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 516a851d3d55..f45972e3c823 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17506,7 +17506,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, &info, supportMaxOptions, - supportGroupNameOption, + set_fields & QEMU_BLOCK_IOTUNE_SET_GROUP_NAME, supportMaxLengthOptions); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1;
I believe this should be a change to qemuMonitorJSONSetBlockIoThrottle to use 'S' instead of 's' in the virJSONValueObjectAdd call.
Been too long to remember why I went with 's' - although I wonder if it had to do with "unsetting" the value... I do believe I should have gone with my first instincts on this and should have made the design around allowing the user to define a group number to use and then convert that into a group name string with a static prefix and the number <sigh>.
The way other options (like _max options, or the _bytes,_iops) work like this *because* they need to be set either all or none. The group_name can be set or not set by itself, no need for other options as there is no clash if you just update that. You also can't set it to what you got from QEMU, because QEMU can change it automatically; if you have no block I/O throttle set at all then there is no group, when you set anything the group name defaults to the id of the device (that way it is unique and it works like there is no group). The same way if you then want to reset it, without changing the group name, you can't set everything to zeroes and then group name to the same string, it will be reset anyway. So group name should've had a different treatment than the other options.
ACK with that change.
John
On 01/25/2017 08:55 AM, Martin Kletzander wrote:
On Wed, Jan 25, 2017 at 06:43:39AM -0500, John Ferlan wrote:
On 01/25/2017 04:16 AM, Martin Kletzander wrote:
We were setting it based on whether it was supported and that lead to setting it to NULL, which our JSON code caught. However it ended up producing the following results:
$ virsh blkdeviotune fedora vda --total-bytes-sec-max 2000 error: Unable to change block I/O throttle error: internal error: argument key 'group' must not have null value
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 516a851d3d55..f45972e3c823 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17506,7 +17506,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, &info, supportMaxOptions, - supportGroupNameOption, + set_fields & QEMU_BLOCK_IOTUNE_SET_GROUP_NAME, supportMaxLengthOptions); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1;
I believe this should be a change to qemuMonitorJSONSetBlockIoThrottle to use 'S' instead of 's' in the virJSONValueObjectAdd call.
Been too long to remember why I went with 's' - although I wonder if it had to do with "unsetting" the value... I do believe I should have gone with my first instincts on this and should have made the design around allowing the user to define a group number to use and then convert that into a group name string with a static prefix and the number <sigh>.
The way other options (like _max options, or the _bytes,_iops) work like this *because* they need to be set either all or none. The group_name can be set or not set by itself, no need for other options as there is no clash if you just update that. You also can't set it to what you got from QEMU, because QEMU can change it automatically; if you have no block I/O throttle set at all then there is no group, when you set anything the group name defaults to the id of the device (that way it is unique and it works like there is no group). The same way if you then want to reset it, without changing the group name, you can't set everything to zeroes and then group name to the same string, it will be reset anyway. So group name should've had a different treatment than the other options.
You have an ACK from Michal for the methodology you've chosen - that's fine - I just think it's cleaner to use the 'S' instead of 's'.
From the aspect of clearing after setting - right for live path that wouldn't make sense, so no sense in allowing that modification anyway.
John
On 01/25/2017 03:18 PM, John Ferlan wrote:
On 01/25/2017 08:55 AM, Martin Kletzander wrote:
On Wed, Jan 25, 2017 at 06:43:39AM -0500, John Ferlan wrote:
On 01/25/2017 04:16 AM, Martin Kletzander wrote:
We were setting it based on whether it was supported and that lead to setting it to NULL, which our JSON code caught. However it ended up producing the following results:
$ virsh blkdeviotune fedora vda --total-bytes-sec-max 2000 error: Unable to change block I/O throttle error: internal error: argument key 'group' must not have null value
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 516a851d3d55..f45972e3c823 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17506,7 +17506,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, &info, supportMaxOptions, - supportGroupNameOption, + set_fields & QEMU_BLOCK_IOTUNE_SET_GROUP_NAME, supportMaxLengthOptions); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1;
I believe this should be a change to qemuMonitorJSONSetBlockIoThrottle to use 'S' instead of 's' in the virJSONValueObjectAdd call.
Been too long to remember why I went with 's' - although I wonder if it had to do with "unsetting" the value... I do believe I should have gone with my first instincts on this and should have made the design around allowing the user to define a group number to use and then convert that into a group name string with a static prefix and the number <sigh>.
The way other options (like _max options, or the _bytes,_iops) work like this *because* they need to be set either all or none. The group_name can be set or not set by itself, no need for other options as there is no clash if you just update that. You also can't set it to what you got from QEMU, because QEMU can change it automatically; if you have no block I/O throttle set at all then there is no group, when you set anything the group name defaults to the id of the device (that way it is unique and it works like there is no group). The same way if you then want to reset it, without changing the group name, you can't set everything to zeroes and then group name to the same string, it will be reset anyway. So group name should've had a different treatment than the other options.
You have an ACK from Michal for the methodology you've chosen - that's fine - I just think it's cleaner to use the 'S' instead of 's'.
We can combine these two approaches, can't we? That way we are double sure that the bug will not occur again :-) Michal
On Thu, Jan 26, 2017 at 12:43:09PM +0100, Michal Privoznik wrote:
On 01/25/2017 03:18 PM, John Ferlan wrote:
On 01/25/2017 08:55 AM, Martin Kletzander wrote:
On Wed, Jan 25, 2017 at 06:43:39AM -0500, John Ferlan wrote:
On 01/25/2017 04:16 AM, Martin Kletzander wrote:
We were setting it based on whether it was supported and that lead to setting it to NULL, which our JSON code caught. However it ended up producing the following results:
$ virsh blkdeviotune fedora vda --total-bytes-sec-max 2000 error: Unable to change block I/O throttle error: internal error: argument key 'group' must not have null value
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 516a851d3d55..f45972e3c823 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17506,7 +17506,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, &info, supportMaxOptions, - supportGroupNameOption, + set_fields & QEMU_BLOCK_IOTUNE_SET_GROUP_NAME, supportMaxLengthOptions); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1;
I believe this should be a change to qemuMonitorJSONSetBlockIoThrottle to use 'S' instead of 's' in the virJSONValueObjectAdd call.
Been too long to remember why I went with 's' - although I wonder if it had to do with "unsetting" the value... I do believe I should have gone with my first instincts on this and should have made the design around allowing the user to define a group number to use and then convert that into a group name string with a static prefix and the number <sigh>.
The way other options (like _max options, or the _bytes,_iops) work like this *because* they need to be set either all or none. The group_name can be set or not set by itself, no need for other options as there is no clash if you just update that. You also can't set it to what you got from QEMU, because QEMU can change it automatically; if you have no block I/O throttle set at all then there is no group, when you set anything the group name defaults to the id of the device (that way it is unique and it works like there is no group). The same way if you then want to reset it, without changing the group name, you can't set everything to zeroes and then group name to the same string, it will be reset anyway. So group name should've had a different treatment than the other options.
You have an ACK from Michal for the methodology you've chosen - that's fine - I just think it's cleaner to use the 'S' instead of 's'.
We can combine these two approaches, can't we? That way we are double sure that the bug will not occur again :-)
OK, I did that now.
Michal
On 01/25/2017 10:16 AM, Martin Kletzander wrote:
We were setting it based on whether it was supported and that lead to setting it to NULL, which our JSON code caught. However it ended up producing the following results:
$ virsh blkdeviotune fedora vda --total-bytes-sec-max 2000 error: Unable to change block I/O throttle error: internal error: argument key 'group' must not have null value
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 516a851d3d55..f45972e3c823 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17506,7 +17506,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, &info, supportMaxOptions, - supportGroupNameOption, + set_fields & QEMU_BLOCK_IOTUNE_SET_GROUP_NAME, supportMaxLengthOptions); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1;
ACK, but the other options (Max and MaxLength) seem to suffer the same issue. Michal
On Wed, Jan 25, 2017 at 02:32:13PM +0100, Michal Privoznik wrote:
On 01/25/2017 10:16 AM, Martin Kletzander wrote:
We were setting it based on whether it was supported and that lead to setting it to NULL, which our JSON code caught. However it ended up producing the following results:
$ virsh blkdeviotune fedora vda --total-bytes-sec-max 2000 error: Unable to change block I/O throttle error: internal error: argument key 'group' must not have null value
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 516a851d3d55..f45972e3c823 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17506,7 +17506,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, &info, supportMaxOptions, - supportGroupNameOption, + set_fields & QEMU_BLOCK_IOTUNE_SET_GROUP_NAME, supportMaxLengthOptions); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1;
ACK, but the other options (Max and MaxLength) seem to suffer the same issue.
I believe that is handled by qemuDomainSetBlockIoTuneDefaults(). If not, patches are welcome :D
Michal
Due to our APIs not copying various pointers, we need to carry it around on the side and just supply it every time it is needed. Otherwise it will not work with both --live and --config options. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f45972e3c823..3a0245ec1cb8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17256,6 +17256,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, virTypedParameterPtr eventParams = NULL; int eventNparams = 0; int eventMaxparams = 0; + char *group_name = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -17370,7 +17371,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, /* NB: Cannot use macro since this is a value.s not a value.ul */ if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME)) { - if (VIR_STRDUP(info.group_name, params->value.s) < 0) + if (VIR_STRDUP(group_name, params->value.s) < 0) goto endjob; set_fields |= QEMU_BLOCK_IOTUNE_SET_GROUP_NAME; if (virTypedParamsAddString(&eventParams, &eventNparams, @@ -17500,6 +17501,10 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, #undef CHECK_MAX + if (set_fields & QEMU_BLOCK_IOTUNE_SET_GROUP_NAME && + VIR_STRDUP(info.group_name, group_name) < 0) + goto endjob; + /* NB: Let's let QEMU decide how to handle issues with _length * via the JSON error code from the block_set_io_throttle call */ @@ -17513,7 +17518,6 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, if (ret < 0) goto endjob; disk->blkdeviotune = info; - info.group_name = NULL; ret = virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps); if (ret < 0) @@ -17533,10 +17537,14 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, path); goto endjob; } + + if (set_fields & QEMU_BLOCK_IOTUNE_SET_GROUP_NAME && + VIR_STRDUP(info.group_name, group_name) < 0) + goto endjob; + qemuDomainSetBlockIoTuneDefaults(&info, &conf_disk->blkdeviotune, set_fields); conf_disk->blkdeviotune = info; - info.group_name = NULL; ret = virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef); if (ret < 0) goto endjob; @@ -17547,7 +17555,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEndJob(driver, vm); cleanup: - VIR_FREE(info.group_name); + VIR_FREE(group_name); VIR_FREE(device); virDomainObjEndAPI(&vm); if (eventNparams) -- 2.11.0
On 01/25/2017 04:16 AM, Martin Kletzander wrote:
Due to our APIs not copying various pointers, we need to carry it around on the side and just supply it every time it is needed. Otherwise it will not work with both --live and --config options.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f45972e3c823..3a0245ec1cb8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17256,6 +17256,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, virTypedParameterPtr eventParams = NULL; int eventNparams = 0; int eventMaxparams = 0; + char *group_name = NULL;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -17370,7 +17371,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
/* NB: Cannot use macro since this is a value.s not a value.ul */ if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME)) { - if (VIR_STRDUP(info.group_name, params->value.s) < 0) + if (VIR_STRDUP(group_name, params->value.s) < 0) goto endjob; set_fields |= QEMU_BLOCK_IOTUNE_SET_GROUP_NAME; if (virTypedParamsAddString(&eventParams, &eventNparams, @@ -17500,6 +17501,10 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
#undef CHECK_MAX
+ if (set_fields & QEMU_BLOCK_IOTUNE_SET_GROUP_NAME && + VIR_STRDUP(info.group_name, group_name) < 0) + goto endjob; +
At this point group_name is either NULL or not depending on the above setting. The VIR_STRDUP will nicely handle a NULL group name by not making the copy, so you can safely remove the "set_fields" portion of the check.
/* NB: Let's let QEMU decide how to handle issues with _length * via the JSON error code from the block_set_io_throttle call */
@@ -17513,7 +17518,6 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, if (ret < 0) goto endjob; disk->blkdeviotune = info; - info.group_name = NULL;
ret = virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps); if (ret < 0) @@ -17533,10 +17537,14 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, path); goto endjob; } + + if (set_fields & QEMU_BLOCK_IOTUNE_SET_GROUP_NAME && + VIR_STRDUP(info.group_name, group_name) < 0) + goto endjob; +
likewise. ACK with those adjustments. John
qemuDomainSetBlockIoTuneDefaults(&info, &conf_disk->blkdeviotune, set_fields); conf_disk->blkdeviotune = info; - info.group_name = NULL; ret = virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef); if (ret < 0) goto endjob; @@ -17547,7 +17555,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEndJob(driver, vm);
cleanup: - VIR_FREE(info.group_name); + VIR_FREE(group_name); VIR_FREE(device); virDomainObjEndAPI(&vm); if (eventNparams)
On 01/25/2017 10:16 AM, Martin Kletzander wrote:
Due to our APIs not copying various pointers, we need to carry it around on the side and just supply it every time it is needed. Otherwise it will not work with both --live and --config options.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f45972e3c823..3a0245ec1cb8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17256,6 +17256,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, virTypedParameterPtr eventParams = NULL; int eventNparams = 0; int eventMaxparams = 0; + char *group_name = NULL;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -17370,7 +17371,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
/* NB: Cannot use macro since this is a value.s not a value.ul */ if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME)) { - if (VIR_STRDUP(info.group_name, params->value.s) < 0) + if (VIR_STRDUP(group_name, params->value.s) < 0) goto endjob; set_fields |= QEMU_BLOCK_IOTUNE_SET_GROUP_NAME; if (virTypedParamsAddString(&eventParams, &eventNparams, @@ -17500,6 +17501,10 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
#undef CHECK_MAX
+ if (set_fields & QEMU_BLOCK_IOTUNE_SET_GROUP_NAME && + VIR_STRDUP(info.group_name, group_name) < 0) + goto endjob; +
There is still one problem. qemuDomainSetBlockIoTuneDefaults steals disk->blkdeviotune->group_name pointer (into info->group_name). In ideal case, when there is no error this is not a problem since the disk->blkdeviotune is going to be replaced with info. However, should something go wrong in between those two lines the group_name is lost.
/* NB: Let's let QEMU decide how to handle issues with _length * via the JSON error code from the block_set_io_throttle call */
@@ -17513,7 +17518,6 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, if (ret < 0) goto endjob; disk->blkdeviotune = info; - info.group_name = NULL;
ret = virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps); if (ret < 0) @@ -17533,10 +17537,14 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, path); goto endjob; } + + if (set_fields & QEMU_BLOCK_IOTUNE_SET_GROUP_NAME && + VIR_STRDUP(info.group_name, group_name) < 0) + goto endjob; + qemuDomainSetBlockIoTuneDefaults(&info, &conf_disk->blkdeviotune, set_fields); conf_disk->blkdeviotune = info; - info.group_name = NULL; ret = virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef); if (ret < 0) goto endjob; @@ -17547,7 +17555,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEndJob(driver, vm);
cleanup: - VIR_FREE(info.group_name); + VIR_FREE(group_name); VIR_FREE(device); virDomainObjEndAPI(&vm); if (eventNparams)
Despite the problem I'm describing above, this is undoubtedly an improvement. ACK Michal
Due to our APIs not copying various pointers, we need to carry it around on the side and just supply it every time it is needed. Otherwise it will not work with both --live and --config options. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f45972e3c823..4ff6b9d46502 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17194,7 +17194,7 @@ qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, if (!(set_fields & QEMU_BLOCK_IOTUNE_SET_SIZE_IOPS)) newinfo->size_iops_sec = oldinfo->size_iops_sec; if (!(set_fields & QEMU_BLOCK_IOTUNE_SET_GROUP_NAME)) - VIR_STEAL_PTR(newinfo->group_name, oldinfo->group_name); + VIR_STRDUP(newinfo->group_name, oldinfo->group_name); /* The length field is handled a bit differently. If not defined/set, * QEMU will default these to 0 or 1 depending on whether something in @@ -17512,8 +17512,10 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, ret = -1; if (ret < 0) goto endjob; + + VIR_FREE(disk->blkdeviotune.group_name); disk->blkdeviotune = info; - info.group_name = NULL; + VIR_STRDUP(disk->blkdeviotune.group_name, info.group_name); ret = virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps); if (ret < 0) @@ -17533,10 +17535,14 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, path); goto endjob; } + qemuDomainSetBlockIoTuneDefaults(&info, &conf_disk->blkdeviotune, set_fields); + + VIR_FREE(conf_disk->blkdeviotune.group_name); conf_disk->blkdeviotune = info; - info.group_name = NULL; + VIR_STRDUP(conf_disk->blkdeviotune.group_name, info.group_name); + ret = virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef); if (ret < 0) goto endjob; -- 2.11.0
On Wed, Jan 25, 2017 at 03:53:20PM +0100, Martin Kletzander wrote:
Due to our APIs not copying various pointers, we need to carry it around on the side and just supply it every time it is needed. Otherwise it will not work with both --live and --config options.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
Scratch this, this needs some fixes as well, but trivial ones, so this whole series is on branch blkdeviotune on my github now https://github.com/nertpinx/libvirt/tree/blkdeviotune
On 26.01.2017 15:38, Martin Kletzander wrote:
On Wed, Jan 25, 2017 at 03:53:20PM +0100, Martin Kletzander wrote:
Due to our APIs not copying various pointers, we need to carry it around on the side and just supply it every time it is needed. Otherwise it will not work with both --live and --config options.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
Scratch this, this needs some fixes as well, but trivial ones, so this whole series is on branch blkdeviotune on my github now https://github.com/nertpinx/libvirt/tree/blkdeviotune
While it would be nice to send the patches to the list as another version, this has undergone more rounds than needed. ACK to the patches on your branch. There are still some problems (e.g. in "qemu: Don't lose group_name" - we firstly free() and then try to strdup() which can lead to losing the original value if strdup() fails), but those can be fixed after your patches are pushed. Michal
On Sat, Jan 28, 2017 at 10:03:13AM +0100, Michal Privoznik wrote:
On 26.01.2017 15:38, Martin Kletzander wrote:
On Wed, Jan 25, 2017 at 03:53:20PM +0100, Martin Kletzander wrote:
Due to our APIs not copying various pointers, we need to carry it around on the side and just supply it every time it is needed. Otherwise it will not work with both --live and --config options.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
Scratch this, this needs some fixes as well, but trivial ones, so this whole series is on branch blkdeviotune on my github now https://github.com/nertpinx/libvirt/tree/blkdeviotune
While it would be nice to send the patches to the list as another version, this has undergone more rounds than needed.
ACK to the patches on your branch. There are still some problems (e.g. in "qemu: Don't lose group_name" - we firstly free() and then try to strdup() which can lead to losing the original value if strdup() fails), but those can be fixed after your patches are pushed.
You know what, you're right, I was trying to push this just to get rid of it, but why couldn't we do this the tight way, right? I rebase the branch on master, dropped the "don't lose group_name" commit and I'll resend that one. I'll also push the rest, so it's just one clean patch and not resends of stuff. Who knows how many other things we'll end up changing when hunting down the one issue.
Michal
Function vshCommandOptStringReq() returns -1 on error and 0 on success. The code, however, used the 'group_name' variable only if it returned 1 (never). Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tools/virsh-domain.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 93587e8bc7c7..4360306989d8 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1398,11 +1398,12 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) VSH_ADD_IOTUNE(write-iops-sec-max-length, WRITE_IOPS_SEC_MAX_LENGTH); #undef VSH_ADD_IOTUNE - rv = vshCommandOptStringReq(ctl, cmd, "group_name", &group_name); - if (rv < 0) { + if (vshCommandOptStringReq(ctl, cmd, "group_name", &group_name) < 0) { vshError(ctl, "%s", _("Unable to parse group parameter")); goto cleanup; - } else if (rv > 0) { + } + + if (group_name) { if (virTypedParamsAddString(¶ms, &nparams, &maxparams, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME, group_name) < 0) -- 2.11.0
On 01/25/2017 04:16 AM, Martin Kletzander wrote:
Function vshCommandOptStringReq() returns -1 on error and 0 on success. The code, however, used the 'group_name' variable only if it returned 1 (never).
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tools/virsh-domain.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
Ugh... ACK John .
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 93587e8bc7c7..4360306989d8 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1398,11 +1398,12 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) VSH_ADD_IOTUNE(write-iops-sec-max-length, WRITE_IOPS_SEC_MAX_LENGTH); #undef VSH_ADD_IOTUNE
- rv = vshCommandOptStringReq(ctl, cmd, "group_name", &group_name); - if (rv < 0) { + if (vshCommandOptStringReq(ctl, cmd, "group_name", &group_name) < 0) { vshError(ctl, "%s", _("Unable to parse group parameter"));
^^ Ah.. remnant of my original thoughts - this was going to a "group #" argument and a last minute change to use a string (of course that was sometime in Sept/Oct for the v1 patches). Still I cannot recall (now) how I wouldn't have noticed this while testing.
goto cleanup; - } else if (rv > 0) { + } + + if (group_name) { if (virTypedParamsAddString(¶ms, &nparams, &maxparams, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME, group_name) < 0)
On 01/25/2017 10:16 AM, Martin Kletzander wrote:
Function vshCommandOptStringReq() returns -1 on error and 0 on success. The code, however, used the 'group_name' variable only if it returned 1 (never).
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tools/virsh-domain.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
ACK Michal
All options started with underscores, but we switched them to dashes later on, making the style consistent. The latest addition, however, did not respect that, so let's change that as well. It is tempting to just change the name instead of adding alias, especially since nobody ever used it, which we know thanks to the fact that it didn't work. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tools/virsh-domain.c | 6 +++++- tools/virsh.pod | 4 ++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 4360306989d8..4f7f23f8a18e 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1264,6 +1264,10 @@ static const vshCmdOptDef opts_blkdeviotune[] = { .help = N_("I/O size in bytes") }, {.name = "group_name", + .type = VSH_OT_ALIAS, + .help = "group-name" + }, + {.name = "group-name", .type = VSH_OT_STRING, .help = N_("group name to share I/O quota between multiple drives") }, @@ -1398,7 +1402,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) VSH_ADD_IOTUNE(write-iops-sec-max-length, WRITE_IOPS_SEC_MAX_LENGTH); #undef VSH_ADD_IOTUNE - if (vshCommandOptStringReq(ctl, cmd, "group_name", &group_name) < 0) { + if (vshCommandOptStringReq(ctl, cmd, "group-name", &group_name) < 0) { vshError(ctl, "%s", _("Unable to parse group parameter")); goto cleanup; } diff --git a/tools/virsh.pod b/tools/virsh.pod index 290f5083d99c..8512ff136265 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1139,7 +1139,7 @@ command. [I<read-bytes-sec-max-length>] [I<write-bytes-sec-max-length>]] [[I<total-iops-sec-max-length>] | [I<read-iops-sec-max-length>] [I<write-iops-sec-max-length>]] -[I<size-iops-sec>] [I<group_name>] +[I<size-iops-sec>] [I<group-name>] Set or query the block disk io parameters for a block device of I<domain>. I<device> specifies a unique target name (<target dev='name'/>) or source @@ -1179,7 +1179,7 @@ read I/O operations limit. I<--write-iops-sec-max-length> specifies duration in seconds to allow maximum write I/O operations limit. I<--size-iops-sec> specifies size I/O operations limit per second. -I<--group_name> specifies group name to share I/O quota between multiple drives. +I<--group-name> specifies group name to share I/O quota between multiple drives. For a qemu domain, if no name is provided, then the default is to have a single group for each I<device>. -- 2.11.0
On 01/25/2017 04:16 AM, Martin Kletzander wrote:
All options started with underscores, but we switched them to dashes later on, making the style consistent. The latest addition, however, did not respect that, so let's change that as well. It is tempting to just change the name instead of adding alias, especially since nobody ever used it, which we know thanks to the fact that it didn't work.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tools/virsh-domain.c | 6 +++++- tools/virsh.pod | 4 ++-- 2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 4360306989d8..4f7f23f8a18e 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1264,6 +1264,10 @@ static const vshCmdOptDef opts_blkdeviotune[] = { .help = N_("I/O size in bytes") }, {.name = "group_name", + .type = VSH_OT_ALIAS, + .help = "group-name" + }, + {.name = "group-name", .type = VSH_OT_STRING, .help = N_("group name to share I/O quota between multiple drives") }, @@ -1398,7 +1402,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) VSH_ADD_IOTUNE(write-iops-sec-max-length, WRITE_IOPS_SEC_MAX_LENGTH); #undef VSH_ADD_IOTUNE
- if (vshCommandOptStringReq(ctl, cmd, "group_name", &group_name) < 0) { + if (vshCommandOptStringReq(ctl, cmd, "group-name", &group_name) < 0) { vshError(ctl, "%s", _("Unable to parse group parameter"));
Perhaps here's where the error message can change to be 'group-name' instead of 'group' ACK - (whether you adjust the error message or not) John
goto cleanup; } diff --git a/tools/virsh.pod b/tools/virsh.pod index 290f5083d99c..8512ff136265 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1139,7 +1139,7 @@ command. [I<read-bytes-sec-max-length>] [I<write-bytes-sec-max-length>]] [[I<total-iops-sec-max-length>] | [I<read-iops-sec-max-length>] [I<write-iops-sec-max-length>]] -[I<size-iops-sec>] [I<group_name>] +[I<size-iops-sec>] [I<group-name>]
Set or query the block disk io parameters for a block device of I<domain>. I<device> specifies a unique target name (<target dev='name'/>) or source @@ -1179,7 +1179,7 @@ read I/O operations limit. I<--write-iops-sec-max-length> specifies duration in seconds to allow maximum write I/O operations limit. I<--size-iops-sec> specifies size I/O operations limit per second. -I<--group_name> specifies group name to share I/O quota between multiple drives. +I<--group-name> specifies group name to share I/O quota between multiple drives. For a qemu domain, if no name is provided, then the default is to have a single group for each I<device>.
On 01/25/2017 10:16 AM, Martin Kletzander wrote:
All options started with underscores, but we switched them to dashes later on, making the style consistent. The latest addition, however, did not respect that, so let's change that as well. It is tempting to just change the name instead of adding alias, especially since nobody ever used it, which we know thanks to the fact that it didn't work.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tools/virsh-domain.c | 6 +++++- tools/virsh.pod | 4 ++-- 2 files changed, 7 insertions(+), 3 deletions(-)
ACK Michal
For example when both total_bytes_sec and total_bytes_sec_max are set, but the former gets cleaned due to new call setting, let's say, read_bytes_sec, we end up with this weird message for the command: $ virsh blkdeviotune fedora vda --read-bytes-sec 3000 error: Unable to change block I/O throttle error: unsupported configuration: value 'total_bytes_sec_max' cannot be set if 'total_bytes_sec' is not set So let's make it more descriptive. This is how it looks after the change: $ virsh blkdeviotune fedora vda --read-bytes-sec 3000 error: Unable to change block I/O throttle error: unsupported configuration: cannot reset 'total_bytes_sec' when 'total_bytes_sec_max' is set Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1344897 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 46 +++++++++++++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3a0245ec1cb8..82dda8db8f86 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17481,23 +17481,39 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainSetBlockIoTuneDefaults(&info, &disk->blkdeviotune, set_fields); -#define CHECK_MAX(val) \ +#define CHECK_MAX(val, _bool) \ do { \ - if (info.val##_max && !info.val) { \ - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \ - _("value '%s' cannot be set if " \ - "'%s' is not set"), \ - #val "_max", #val); \ - goto endjob; \ + if (info.val##_max) { \ + if (!info.val) { \ + if (QEMU_BLOCK_IOTUNE_SET_##_bool) { \ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \ + _("cannot reset '%s' when " \ + "'%s' is set"), \ + #val, #val "_max"); \ + } else { \ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \ + _("value '%s' cannot be set if " \ + "'%s' is not set"), \ + #val "_max", #val); \ + } \ + goto endjob; \ + } \ + if (info.val##_max < info.val) { \ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \ + _("value '%s' cannot be " \ + "smaller than '%s'"), \ + #val "_max", #val); \ + goto endjob; \ + } \ } \ - } while (false); - - CHECK_MAX(total_bytes_sec); - CHECK_MAX(read_bytes_sec); - CHECK_MAX(write_bytes_sec); - CHECK_MAX(total_iops_sec); - CHECK_MAX(read_iops_sec); - CHECK_MAX(write_iops_sec); + } while (false) + + CHECK_MAX(total_bytes_sec, BYTES); + CHECK_MAX(read_bytes_sec, BYTES); + CHECK_MAX(write_bytes_sec, BYTES); + CHECK_MAX(total_iops_sec, IOPS); + CHECK_MAX(read_iops_sec, IOPS); + CHECK_MAX(write_iops_sec, IOPS); #undef CHECK_MAX -- 2.11.0
On 01/25/2017 04:16 AM, Martin Kletzander wrote:
For example when both total_bytes_sec and total_bytes_sec_max are set, but the former gets cleaned due to new call setting, let's say, read_bytes_sec, we end up with this weird message for the command:
$ virsh blkdeviotune fedora vda --read-bytes-sec 3000 error: Unable to change block I/O throttle error: unsupported configuration: value 'total_bytes_sec_max' cannot be set if 'total_bytes_sec' is not set
So let's make it more descriptive. This is how it looks after the change:
$ virsh blkdeviotune fedora vda --read-bytes-sec 3000 error: Unable to change block I/O throttle error: unsupported configuration: cannot reset 'total_bytes_sec' when 'total_bytes_sec_max' is set
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1344897
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 46 +++++++++++++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 15 deletions(-)
At least this is easier with those macros... I'm fine with the error message adjustments here - although I thought QEMU checked the various inconsistencies and messaged based on that (perhaps something I checked when using the qemu command line instead of via virsh - cannot recall now). ACK, John oh and before I forget... Could you please update: https://bugzilla.redhat.com/show_bug.cgi?id=1336564 to indicate which commit fixes the issues for group name. Thanks and sorry for the mess.
On Wed, Jan 25, 2017 at 07:38:26AM -0500, John Ferlan wrote:
On 01/25/2017 04:16 AM, Martin Kletzander wrote:
For example when both total_bytes_sec and total_bytes_sec_max are set, but the former gets cleaned due to new call setting, let's say, read_bytes_sec, we end up with this weird message for the command:
$ virsh blkdeviotune fedora vda --read-bytes-sec 3000 error: Unable to change block I/O throttle error: unsupported configuration: value 'total_bytes_sec_max' cannot be set if 'total_bytes_sec' is not set
So let's make it more descriptive. This is how it looks after the change:
$ virsh blkdeviotune fedora vda --read-bytes-sec 3000 error: Unable to change block I/O throttle error: unsupported configuration: cannot reset 'total_bytes_sec' when 'total_bytes_sec_max' is set
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1344897
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 46 +++++++++++++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 15 deletions(-)
At least this is easier with those macros... I'm fine with the error message adjustments here - although I thought QEMU checked the various inconsistencies and messaged based on that (perhaps something I checked when using the qemu command line instead of via virsh - cannot recall now).
It does, but we don't handle all types of errors very well. Also it's better to error out earlier, what if some version of QEMU will set half of the settings before failing? Anyway, QEMU checked that, but it ended up like this (can be triggerredfor example by setting *bytes_sec to more than *bytes_sec_max): $ virsh blkdeviotune vm2 vda --total-bytes-sec 3000 error: Unable to change block I/O throttle error: internal error: Unexpected error See the BZ for details. I didn't want to make the commit messages longer than the (self-describing) patches themselves.
ACK,
John
oh and before I forget... Could you please update:
https://bugzilla.redhat.com/show_bug.cgi?id=1336564
to indicate which commit fixes the issues for group name. Thanks and sorry for the mess.
On 01/25/2017 09:07 AM, Martin Kletzander wrote:
On Wed, Jan 25, 2017 at 07:38:26AM -0500, John Ferlan wrote:
On 01/25/2017 04:16 AM, Martin Kletzander wrote:
For example when both total_bytes_sec and total_bytes_sec_max are set, but the former gets cleaned due to new call setting, let's say, read_bytes_sec, we end up with this weird message for the command:
$ virsh blkdeviotune fedora vda --read-bytes-sec 3000 error: Unable to change block I/O throttle error: unsupported configuration: value 'total_bytes_sec_max' cannot be set if 'total_bytes_sec' is not set
So let's make it more descriptive. This is how it looks after the change:
$ virsh blkdeviotune fedora vda --read-bytes-sec 3000 error: Unable to change block I/O throttle error: unsupported configuration: cannot reset 'total_bytes_sec' when 'total_bytes_sec_max' is set
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1344897
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 46 +++++++++++++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 15 deletions(-)
At least this is easier with those macros... I'm fine with the error message adjustments here - although I thought QEMU checked the various inconsistencies and messaged based on that (perhaps something I checked when using the qemu command line instead of via virsh - cannot recall now).
It does, but we don't handle all types of errors very well. Also it's better to error out earlier, what if some version of QEMU will set half of the settings before failing?
I recall running into "odd" errors while adding the _max changes, but since QEMU messaged them I think I felt at the time it would be better to punt and let QEMU handle it rather than checking in libvirt. I do agree though - I'd rather see libvirt code check for these types of errors, but I also know I've done that in other code and have had review comments that essentially said - let QEMU handle that and use the QEMU message.
Anyway, QEMU checked that, but it ended up like this (can be triggerredfor example by setting *bytes_sec to more than *bytes_sec_max):
$ virsh blkdeviotune vm2 vda --total-bytes-sec 3000 error: Unable to change block I/O throttle error: internal error: Unexpected error
See the BZ for details. I didn't want to make the commit messages longer than the (self-describing) patches themselves.
I quickly scanned the bz... For some reason I thought though that I was getting at least some message back from qemu in 'later' libvirt code rather than that Unexpected error... as if some other patch had made sure to propagate the qemu message. John
ACK,
John
oh and before I forget... Could you please update:
https://bugzilla.redhat.com/show_bug.cgi?id=1336564
to indicate which commit fixes the issues for group name. Thanks and sorry for the mess.
On 01/25/2017 10:16 AM, Martin Kletzander wrote:
For example when both total_bytes_sec and total_bytes_sec_max are set, but the former gets cleaned due to new call setting, let's say, read_bytes_sec, we end up with this weird message for the command:
$ virsh blkdeviotune fedora vda --read-bytes-sec 3000 error: Unable to change block I/O throttle error: unsupported configuration: value 'total_bytes_sec_max' cannot be set if 'total_bytes_sec' is not set
So let's make it more descriptive. This is how it looks after the change:
$ virsh blkdeviotune fedora vda --read-bytes-sec 3000 error: Unable to change block I/O throttle error: unsupported configuration: cannot reset 'total_bytes_sec' when 'total_bytes_sec_max' is set
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1344897
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 46 +++++++++++++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 15 deletions(-)
ACK Michal
participants (3)
-
John Ferlan -
Martin Kletzander -
Michal Privoznik