Re: [PATCH] qemu: Fix qemuMonitorCreateObjectProps

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Jano

Attachment: signature.asc
Description: Digital signature

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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux