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