3:16 a.m.
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
3:16 a.m.
New subject: [libvirt] [PATCH 1/5] qemu: Only set group_name when actually requested
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(a)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
5:43 a.m.
New subject: [libvirt] [PATCH 1/5] qemu: Only set group_name when actually requested
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(a)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
7:55 a.m.
New subject: [libvirt] [PATCH 1/5] qemu: Only set group_name when actually requested
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(a)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
8:18 a.m.
New subject: [libvirt] [PATCH 1/5] qemu: Only set group_name when actually requested
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(a)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
5:43 a.m.
New subject: [libvirt] [PATCH 1/5] qemu: Only set group_name when actually requested
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(a)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
8:18 a.m.
New subject: [libvirt] [PATCH 1/5] qemu: Only set group_name when actually requested
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(a)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
7:32 a.m.
New subject: [libvirt] [PATCH 1/5] qemu: Only set group_name when actually requested
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(a)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
7:55 a.m.
New subject: [libvirt] [PATCH 1/5] qemu: Only set group_name when actually requested
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(a)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
3:16 a.m.
New subject: [libvirt] [PATCH 2/5] qemu: Don't lose group_name
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(a)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
5:46 a.m.
New subject: [libvirt] [PATCH 2/5] qemu: Don't lose group_name
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(a)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)
7:32 a.m.
New subject: [libvirt] [PATCH 2/5] qemu: Don't lose group_name
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(a)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
8:53 a.m.
New subject: [libvirt] [PATCH v2] qemu: Don't lose group_name
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(a)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
8:38 a.m.
New subject: [libvirt] [PATCH v2] qemu: Don't lose group_name
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(a)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
3:03 a.m.
New subject: [libvirt] [PATCH v2] qemu: Don't lose group_name
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(a)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
12:57 p.m.
New subject: [libvirt] [PATCH v2] qemu: Don't lose group_name
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(a)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
3:16 a.m.
New subject: [libvirt] [PATCH 3/5] virsh: Actually make blkdeviotune --group_name work
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(a)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
6:04 a.m.
New subject: [libvirt] [PATCH 3/5] virsh: Actually make blkdeviotune --group_name work
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(a)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)
7:32 a.m.
New subject: [libvirt] [PATCH 3/5] virsh: Actually make blkdeviotune --group_name work
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(a)redhat.com>
---
tools/virsh-domain.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
ACK
Michal
3:16 a.m.
New subject: [libvirt] [PATCH 4/5] virsh: Use consistent naming for blkdeviotune options
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(a)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
6:10 a.m.
New subject: [libvirt] [PATCH 4/5] virsh: Use consistent naming for blkdeviotune options
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(a)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>.
7:32 a.m.
New subject: [libvirt] [PATCH 4/5] virsh: Use consistent naming for blkdeviotune options
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(a)redhat.com>
---
tools/virsh-domain.c | 6 +++++-
tools/virsh.pod | 4 ++--
2 files changed, 7 insertions(+), 3 deletions(-)
ACK
Michal
3:16 a.m.
New subject: [libvirt] [PATCH 5/5] qemu: Add better message for some invalid block I/O settings
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(a)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
6:38 a.m.
New subject: [libvirt] [PATCH 5/5] qemu: Add better message for some invalid block I/O settings
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(a)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.
8:07 a.m.
New subject: [libvirt] [PATCH 5/5] qemu: Add better message for some invalid block I/O settings
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(a)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.
8:24 a.m.
New subject: [libvirt] [PATCH 5/5] qemu: Add better message for some invalid block I/O settings
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(a)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.
>
7:31 a.m.
New subject: [libvirt] [PATCH 5/5] qemu: Add better message for some invalid block I/O settings
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(a)redhat.com>
---
src/qemu/qemu_driver.c | 46 +++++++++++++++++++++++++++++++---------------
1 file changed, 31 insertions(+), 15 deletions(-)
ACK
Michal
2854
days inactive
2858
days old
26 comments
3 participants
participants (3)
-
John Ferlan -
Martin Kletzander -
Michal Privoznik