6:56 a.m.
Commit id f0a23c0c3 introduced qemuMonitorCreateObjectProps
to manage wrapping the object name and alias; however, calling
virJSONValueObjectCreateVArgs and checking a unary ! return
status meant when the CreateVAArgs function returned zero, then
rather than generating a @propsret, the jump to cleanup would
result in an unexpected failure and cause virsh to issue the
"error: An error occurred, but the cause is unknown" error
message for something like "virsh iothreadadd $DOM 10" (assuming
that IOThread 10 didn't already exist).
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_monitor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index b29672d4f1..d6771c1d52 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3051,7 +3051,7 @@ qemuMonitorCreateObjectProps(virJSONValuePtr *propsret,
va_start(args, alias);
- if (!(virJSONValueObjectCreateVArgs(&props, args)))
+ if (virJSONValueObjectCreateVArgs(&props, args) < 0)
goto cleanup;
if (!(*propsret = qemuMonitorCreateObjectPropsWrap(type, alias, &props)))
--
2.14.4
7:35 a.m.
The commit message is one long sentence and a bit hard to read.
On Mon, Jun 18, 2018 at 07:56:35AM -0400, John Ferlan wrote:
Commit id f0a23c0c3 introduced qemuMonitorCreateObjectProps
to manage wrapping the object name and alias; however, calling
Listing the commit that broke it can be in a separate paragraph.
virJSONValueObjectCreateVArgs and checking a unary ! return
status meant when the CreateVAArgs function returned zero, then
rather than generating a @propsret, the jump to cleanup would
Rather than explaing the semantics of C operators, it would be helpful
to mention that virJSONValueObjectCreateVArgs returns -1 on failure
and 0 when we did not need to add any arguments.
result in an unexpected failure and cause virsh to issue the
"error: An error occurred, but the cause is unknown" error
message for something like "virsh iothreadadd $DOM 10" (assuming
that IOThread 10 didn't already exist).
Putting the example error message and virsh commands on separate lines
would make the commit message easier to read.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_monitor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index b29672d4f1..d6771c1d52 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3051,7 +3051,7 @@ qemuMonitorCreateObjectProps(virJSONValuePtr *propsret,
va_start(args, alias);
- if (!(virJSONValueObjectCreateVArgs(&props, args)))
+ if (virJSONValueObjectCreateVArgs(&props, args) < 0)
Looks like qemuDomainHotplugAddIOThread is the only user of qemuMonitorCreateObjectProps
passing NULL for args. Can it be tested by our test suite?
With more newlines in the commit message:
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:34 a.m.
On 06/18/2018 08:35 AM, Ján Tomko wrote:
The commit message is one long sentence and a bit hard to read.
True, haha so it is... Still better than nothing and assuming the reader
can figure it out ;-)
On Mon, Jun 18, 2018 at 07:56:35AM -0400, John Ferlan wrote:
> Commit id f0a23c0c3 introduced qemuMonitorCreateObjectProps
> to manage wrapping the object name and alias; however, calling
Listing the commit that broke it can be in a separate paragraph.
> virJSONValueObjectCreateVArgs and checking a unary ! return
> status meant when the CreateVAArgs function returned zero, then
> rather than generating a @propsret, the jump to cleanup would
Rather than explaing the semantics of C operators, it would be helpful
to mention that virJSONValueObjectCreateVArgs returns -1 on failure
and 0 when we did not need to add any arguments.
6 of one... < 0 is just a C semantic check too of a different style. The
unary check for an integer which can be == 0 is just one of those things
that I see a lot in code and I usually point out much to the dismay of a
few people.
> result in an unexpected failure and cause virsh to issue the
> "error: An error occurred, but the cause is unknown" error
> message for something like "virsh iothreadadd $DOM 10" (assuming
> that IOThread 10 didn't already exist).
Putting the example error message and virsh commands on separate lines
would make the commit message easier to read.
I'll alter to:
Fix the return value status comparison checking for call to
virJSONValueObjectCreateVArgs introduced by commit id f0a23c0c3.
If a NULL arglist is passed, then a 0 is returned which is a valid
status and we only should fail when the return is < 0.
This resolves an issue seen for "virsh iothreadadd $dom $iothread" where
a "error: An error occurred, but the cause is unknown" error was
generated when trying to hotplug an IOThread to a domain since
qemuDomainHotplugAddIOThread passes a NULL arglist.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/qemu/qemu_monitor.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index b29672d4f1..d6771c1d52 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -3051,7 +3051,7 @@ qemuMonitorCreateObjectProps(virJSONValuePtr
> *propsret,
>
> va_start(args, alias);
>
> - if (!(virJSONValueObjectCreateVArgs(&props, args)))
> + if (virJSONValueObjectCreateVArgs(&props, args) < 0)
Looks like qemuDomainHotplugAddIOThread is the only user of
qemuMonitorCreateObjectProps
passing NULL for args. Can it be tested by our test suite?
Perhaps by anyone that wants to write those tests... maybe the original
author should have done that ;-)!
Thanks for the quick review.
John
With more newlines in the commit message:
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
2349
days inactive
2349
days old
2 comments
2 participants
participants (2)
-
John Ferlan -
Ján Tomko