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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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@redhat.com>
> 
> Jano

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list