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