Re: [PATCH 4/4] qemu: query command line options in QMP

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

 



On 05/13/2013 02:35 AM, Osier Yang wrote:

>> +
>> +    array = qemuMonitorGetOptions(mon);
> 
> No need to get it when the options is cached. How about:
> 
>     if (!(array = qemuMonitorGetOptions(mon))) {
>         .....
>     }

Nicer indeed.

> 
>> +    if ((n = virJSONValueArraySize(array)) < 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                       _("query-command-line-options reply data was
>> not "
> 
> s/was/is/, ? I'm sure that I don't want to talk about English with you
> though. :-)

I was just copying existing messages, such as:

qemuMonitorJSONGetKVMState:
                       _("query-kvm reply was missing return data"));

At any rate, the error is internal, because we only expect to see it if
something goes really wrong with the version of qemu we were talking to
compared to what they documented as their interface, so it will probably
never trigger, and I don't think it's worth changing.

>> +        }
>> +
>> +        if (!(paramlist[i] = strdup(tmp))) {
> 
> This has to be changed to use VIR_STRDUP.

Yep.

>> +
>> +    if (nparams != 2) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       "nparams %d is not 2", nparams);
> 
> This will produce something like "4 is not 2", which is obvious and
> confused. :/
> 
> "'nparams' is not 2" might be better.

Looking at other tests, it's nice to know how the test failed; I'll try:

nparams was %d, expected 2

for this and the other places you called out.

> 
> ACK with the nits fixed.

Here's what I'm squashing in before pushing 1-4, along with a tweak to
the commit message requested by Paolo.  I'll let you take over 5/4 as
part of your series.

diff --git i/src/qemu/qemu_monitor_json.c w/src/qemu/qemu_monitor_json.c
index 0423ec7..0c011d3 100644
--- i/src/qemu/qemu_monitor_json.c
+++ w/src/qemu/qemu_monitor_json.c
@@ -4294,7 +4294,7 @@
qemuMonitorJSONGetCommandLineOptionParameters(qemuMonitorPtr mon,
     /* query-command-line-options has fixed output for a given qemu
      * binary; but since callers want to query parameters for one
      * option at a time, we cache the option list from qemu.  */
-    if (!qemuMonitorGetOptions(mon)) {
+    if (!(array = qemuMonitorGetOptions(mon))) {
         if (!(cmd =
qemuMonitorJSONMakeCommand("query-command-line-options",
                                                NULL)))
             return -1;
@@ -4321,7 +4321,6 @@
qemuMonitorJSONGetCommandLineOptionParameters(qemuMonitorPtr mon,

     ret = -1;

-    array = qemuMonitorGetOptions(mon);
     if ((n = virJSONValueArraySize(array)) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("query-command-line-options reply data was not "
@@ -4375,10 +4374,8 @@
qemuMonitorJSONGetCommandLineOptionParameters(qemuMonitorPtr mon,
             goto cleanup;
         }

-        if (!(paramlist[i] = strdup(tmp))) {
-            virReportOOMError();
+        if (VIR_STRDUP(paramlist[i], tmp) < 0)
             goto cleanup;
-        }
     }

     ret = n;
diff --git i/tests/qemumonitorjsontest.c w/tests/qemumonitorjsontest.c
index c7f0536..acc94ca 100644
--- i/tests/qemumonitorjsontest.c
+++ w/tests/qemumonitorjsontest.c
@@ -527,7 +527,7 @@
testQemuMonitorJSONGetCommandLineOptionParameters(const void *data)

     if (nparams != 2) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
-                       "nparams %d is not 2", nparams);
+                       "nparams was %d, expected 2", nparams);
         goto cleanup;
     }

@@ -535,7 +535,7 @@
testQemuMonitorJSONGetCommandLineOptionParameters(const void *data)
     do {                                                                \
         if (STRNEQ(params[i], (wantname))) {                            \
             virReportError(VIR_ERR_INTERNAL_ERROR,                      \
-                           "name %s is not %s",                         \
+                           "name was %s, expected %s",                  \
                            params[i], (wantname));                      \
             goto cleanup;                                               \
         }                                                               \
@@ -557,7 +557,7 @@
testQemuMonitorJSONGetCommandLineOptionParameters(const void *data)

     if (nparams != 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
-                       "nparams %d is not 0", nparams);
+                       "nparams was %d, expected 0", nparams);
         goto cleanup;
     }
     if (params && params[0]) {
@@ -577,7 +577,7 @@
testQemuMonitorJSONGetCommandLineOptionParameters(const void *data)

     if (nparams != 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
-                       "nparams %d is not 0", nparams);
+                       "nparams was %d, expected 0", nparams);
         goto cleanup;
     }
     if (params && params[0]) {


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP 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]