[PATCH 0/3] qemu: Remove caching in handler for query-command-line-options
Shuffle around where we extract the data, so that we don't have to call the monitor multiple times which necessitates caching. Peter Krempa (3): qemu: monitor: Implement new handlers for 'query-command-line-options' virQEMUCapsProbeQMPCommandLine: Rewrite using qemuMonitorGetCommandLineOptions qemuMonitorGetCommandLineOptionParameters: remove the unused function and helpers src/qemu/qemu_capabilities.c | 34 +++++---- src/qemu/qemu_monitor.c | 34 +-------- src/qemu/qemu_monitor.h | 9 +-- src/qemu/qemu_monitor_json.c | 138 +++++++++-------------------------- src/qemu/qemu_monitor_json.h | 6 +- tests/qemumonitorjsontest.c | 117 ----------------------------- 6 files changed, 58 insertions(+), 280 deletions(-) -- 2.28.0
Add a new set hander for getting the data for 'query-command-line-options' which returns everything at once and lets the caller extract the data. This way we don't need to cache the output of the monitor command for repeated calls. Note that we will have enough testing of this code path via qemucapabilitiestest. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor.c | 9 +++++++ src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 48 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 1 + 4 files changed, 59 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index f2ed165b22..a60d04f78a 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3873,6 +3873,15 @@ qemuMonitorGetCommandLineOptionParameters(qemuMonitorPtr mon, } +GHashTable * +qemuMonitorGetCommandLineOptions(qemuMonitorPtr mon) +{ + QEMU_CHECK_MONITOR_NULL(mon); + + return qemuMonitorJSONGetCommandLineOptions(mon); +} + + int qemuMonitorGetKVMState(qemuMonitorPtr mon, bool *enabled, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index d301568e40..6b2e435f70 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1288,6 +1288,7 @@ int qemuMonitorGetCommandLineOptionParameters(qemuMonitorPtr mon, const char *option, char ***params, bool *found); +GHashTable *qemuMonitorGetCommandLineOptions(qemuMonitorPtr mon); int qemuMonitorGetKVMState(qemuMonitorPtr mon, bool *enabled, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 723bdb4426..44d0152812 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6402,6 +6402,54 @@ int qemuMonitorJSONGetEvents(qemuMonitorPtr mon, } +static int +qemuMonitorJSONGetCommandLineOptionsWorker(size_t pos G_GNUC_UNUSED, + virJSONValuePtr item, + void *opaque) +{ + const char *name = virJSONValueObjectGetString(item, "option"); + g_autoptr(virJSONValue) parameters = NULL; + GHashTable *options = opaque; + + if (!name || + virJSONValueObjectRemoveKey(item, "parameters", ¶meters) <= 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("reply data was missing 'option' name or parameters")); + return -1; + } + + g_hash_table_insert(options, g_strdup(name), parameters); + parameters = NULL; + + return 0; +} + + +GHashTable * +qemuMonitorJSONGetCommandLineOptions(qemuMonitorPtr mon) +{ + g_autoptr(GHashTable) ret = virHashNew(virJSONValueHashFree); + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("query-command-line-options", NULL))) + return NULL; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + return NULL; + + if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) + return NULL; + + if (virJSONValueArrayForeachSteal(virJSONValueObjectGetArray(reply, "return"), + qemuMonitorJSONGetCommandLineOptionsWorker, + ret) < 0) + return NULL; + + return g_steal_pointer(&ret); +} + + int qemuMonitorJSONGetCommandLineOptionParameters(qemuMonitorPtr mon, const char *option, diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index b588722d90..53445b97bb 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -430,6 +430,7 @@ int qemuMonitorJSONGetCommandLineOptionParameters(qemuMonitorPtr mon, char ***params, bool *found) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +GHashTable *qemuMonitorJSONGetCommandLineOptions(qemuMonitorPtr mon); int qemuMonitorJSONGetKVMState(qemuMonitorPtr mon, bool *enabled, -- 2.28.0
Use the new handler to fetch the required data and do the extraction locally without conversion to string list. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 538551e772..584bd21be3 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -3226,28 +3226,32 @@ static int virQEMUCapsProbeQMPCommandLine(virQEMUCapsPtr qemuCaps, qemuMonitorPtr mon) { - bool found = false; - int nvalues; - char **values; - size_t i, j; + g_autoptr(GHashTable) options = NULL; + size_t i; + + if (!(options = qemuMonitorGetCommandLineOptions(mon))) + return -1; for (i = 0; i < G_N_ELEMENTS(virQEMUCapsCommandLine); i++) { - if ((nvalues = qemuMonitorGetCommandLineOptionParameters(mon, - virQEMUCapsCommandLine[i].option, - &values, - &found)) < 0) - return -1; + virJSONValuePtr option = g_hash_table_lookup(options, virQEMUCapsCommandLine[i].option); + size_t j; - if (found && !virQEMUCapsCommandLine[i].param) + if (!option) + continue; + + /* not looking for a specific argument */ + if (!virQEMUCapsCommandLine[i].param) { virQEMUCapsSet(qemuCaps, virQEMUCapsCommandLine[i].flag); + continue; + } - for (j = 0; j < nvalues; j++) { - if (STREQ_NULLABLE(virQEMUCapsCommandLine[i].param, values[j])) { + for (j = 0; j < virJSONValueArraySize(option); j++) { + virJSONValuePtr param = virJSONValueArrayGet(option, j); + const char *paramname = virJSONValueObjectGetString(param, "name"); + + if (STREQ_NULLABLE(virQEMUCapsCommandLine[i].param, paramname)) virQEMUCapsSet(qemuCaps, virQEMUCapsCommandLine[i].flag); - break; - } } - g_strfreev(values); } return 0; -- 2.28.0
Remove the function along with helpers for caching the reply and tests. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor.c | 35 ---------- src/qemu/qemu_monitor.h | 8 --- src/qemu/qemu_monitor_json.c | 120 ----------------------------------- src/qemu/qemu_monitor_json.h | 5 -- tests/qemumonitorjsontest.c | 117 ---------------------------------- 5 files changed, 285 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index a60d04f78a..c333fc1364 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -101,9 +101,6 @@ struct _qemuMonitor { bool waitGreeting; - /* cache of query-command-line-options results */ - virJSONValuePtr options; - /* If found, path to the virtio memballoon driver */ char *balloonpath; bool ballooninit; @@ -240,7 +237,6 @@ qemuMonitorDispose(void *obj) virResetError(&mon->lastError); virCondDestroy(&mon->notify); VIR_FREE(mon->buffer); - virJSONValueFree(mon->options); VIR_FREE(mon->balloonpath); } @@ -992,20 +988,6 @@ qemuMonitorLastError(qemuMonitorPtr mon) } -virJSONValuePtr -qemuMonitorGetOptions(qemuMonitorPtr mon) -{ - return mon->options; -} - - -void -qemuMonitorSetOptions(qemuMonitorPtr mon, virJSONValuePtr options) -{ - mon->options = options; -} - - /** * Search the qom objects for the balloon driver object by its known names * of "virtio-balloon-pci" or "virtio-balloon-ccw". The entry for the driver @@ -3856,23 +3838,6 @@ qemuMonitorGetEvents(qemuMonitorPtr mon, } -/* Collect the parameters associated with a given command line option. - * Return count of known parameters or -1 on error. */ -int -qemuMonitorGetCommandLineOptionParameters(qemuMonitorPtr mon, - const char *option, - char ***params, - bool *found) -{ - VIR_DEBUG("option=%s params=%p", option, params); - - QEMU_CHECK_MONITOR(mon); - - return qemuMonitorJSONGetCommandLineOptionParameters(mon, option, - params, found); -} - - GHashTable * qemuMonitorGetCommandLineOptions(qemuMonitorPtr mon) { diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 6b2e435f70..3dcceffef8 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -444,10 +444,6 @@ int qemuMonitorSetLink(qemuMonitorPtr mon, char *qemuMonitorNextCommandID(qemuMonitorPtr mon); int qemuMonitorSend(qemuMonitorPtr mon, qemuMonitorMessagePtr msg) G_GNUC_NO_INLINE; -virJSONValuePtr qemuMonitorGetOptions(qemuMonitorPtr mon) - ATTRIBUTE_NONNULL(1); -void qemuMonitorSetOptions(qemuMonitorPtr mon, virJSONValuePtr options) - ATTRIBUTE_NONNULL(1); int qemuMonitorUpdateVideoMemorySize(qemuMonitorPtr mon, virDomainVideoDefPtr video, const char *videoName) @@ -1284,10 +1280,6 @@ int qemuMonitorGetCommands(qemuMonitorPtr mon, char ***commands); int qemuMonitorGetEvents(qemuMonitorPtr mon, char ***events); -int qemuMonitorGetCommandLineOptionParameters(qemuMonitorPtr mon, - const char *option, - char ***params, - bool *found); GHashTable *qemuMonitorGetCommandLineOptions(qemuMonitorPtr mon); int qemuMonitorGetKVMState(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 44d0152812..5990163519 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6450,126 +6450,6 @@ qemuMonitorJSONGetCommandLineOptions(qemuMonitorPtr mon) } -int -qemuMonitorJSONGetCommandLineOptionParameters(qemuMonitorPtr mon, - const char *option, - char ***params, - bool *found) -{ - int ret = -1; - virJSONValuePtr cmd = NULL; - virJSONValuePtr reply = NULL; - virJSONValuePtr data = NULL; - virJSONValuePtr array = NULL; - char **paramlist = NULL; - size_t n = 0; - size_t i; - - *params = NULL; - if (found) - *found = false; - - /* 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 (!(array = qemuMonitorGetOptions(mon))) { - if (!(cmd = qemuMonitorJSONMakeCommand("query-command-line-options", - NULL))) - return -1; - - if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) - goto cleanup; - - if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { - ret = 0; - goto cleanup; - } - - if (qemuMonitorJSONCheckError(cmd, reply) < 0) - goto cleanup; - - if (virJSONValueObjectRemoveKey(reply, "return", &array) <= 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("query-command-line-options reply was missing " - "return data")); - goto cleanup; - } - - if (!virJSONValueIsArray(array)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Malformed query-command-line-options array")); - goto cleanup; - } - - qemuMonitorSetOptions(mon, array); - } - - for (i = 0; i < virJSONValueArraySize(array); i++) { - virJSONValuePtr child = virJSONValueArrayGet(array, i); - const char *tmp; - - if (!(tmp = virJSONValueObjectGetString(child, "option"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("query-command-line-options reply data was " - "missing 'option'")); - goto cleanup; - } - if (STREQ(tmp, option)) { - data = virJSONValueObjectGet(child, "parameters"); - break; - } - } - - if (!data) { - /* Option not found; return 0 parameters rather than an error. */ - ret = 0; - goto cleanup; - } - - if (found) - *found = true; - - if (!virJSONValueIsArray(data)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Malformed query-command-line-options parameters array")); - goto cleanup; - } - n = virJSONValueArraySize(data); - - /* null-terminated list */ - paramlist = g_new0(char *, n + 1); - - for (i = 0; i < n; i++) { - virJSONValuePtr child = virJSONValueArrayGet(data, i); - const char *tmp; - - if (!(tmp = virJSONValueObjectGetString(child, "name"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("query-command-line-options parameter data was " - "missing 'name'")); - goto cleanup; - } - - paramlist[i] = g_strdup(tmp); - } - - ret = n; - *params = paramlist; - paramlist = NULL; - - cleanup: - /* If we failed before getting the JSON array of options, we (try) - * to cache an empty array to speed up the next failure. */ - if (!qemuMonitorGetOptions(mon)) - qemuMonitorSetOptions(mon, virJSONValueNewArray()); - - g_strfreev(paramlist); - virJSONValueFree(cmd); - virJSONValueFree(reply); - return ret; -} - - int qemuMonitorJSONGetKVMState(qemuMonitorPtr mon, bool *enabled, bool *present) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 53445b97bb..da988f0d41 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -425,11 +425,6 @@ int qemuMonitorJSONGetCommands(qemuMonitorPtr mon, int qemuMonitorJSONGetEvents(qemuMonitorPtr mon, char ***events) ATTRIBUTE_NONNULL(2); -int qemuMonitorJSONGetCommandLineOptionParameters(qemuMonitorPtr mon, - const char *option, - char ***params, - bool *found) - ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); GHashTable *qemuMonitorJSONGetCommandLineOptions(qemuMonitorPtr mon); int qemuMonitorJSONGetKVMState(qemuMonitorPtr mon, diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 25ebf06eb0..d22a92d3e1 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -616,122 +616,6 @@ testQemuMonitorJSONGetTPMModels(const void *opaque) } -static int -testQemuMonitorJSONGetCommandLineOptionParameters(const void *opaque) -{ - const testGenericData *data = opaque; - virDomainXMLOptionPtr xmlopt = data->xmlopt; - int ret = -1; - char **params = NULL; - int nparams = 0; - bool found = false; - g_autoptr(qemuMonitorTest) test = NULL; - - if (!(test = qemuMonitorTestNewSchema(xmlopt, data->schema))) - return -1; - - if (qemuMonitorTestAddItem(test, "query-command-line-options", - "{ " - " \"return\": [ " - " {\"parameters\": [], \"option\": \"acpi\" }," - " {\"parameters\": [" - " {\"name\": \"romfile\", " - " \"type\": \"string\"}, " - " {\"name\": \"bootindex\", " - " \"type\": \"number\"}], " - " \"option\": \"option-rom\"}" - " ]" - "}") < 0) - goto cleanup; - - /* present with params */ - if ((nparams = qemuMonitorGetCommandLineOptionParameters(qemuMonitorTestGetMonitor(test), - "option-rom", - ¶ms, - NULL)) < 0) - goto cleanup; - - if (nparams != 2) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "nparams was %d, expected 2", nparams); - goto cleanup; - } - -#define CHECK(i, wantname) \ - do { \ - if (STRNEQ(params[i], (wantname))) { \ - virReportError(VIR_ERR_INTERNAL_ERROR, \ - "name was %s, expected %s", \ - params[i], (wantname)); \ - goto cleanup; \ - } \ - } while (0) - - CHECK(0, "romfile"); - CHECK(1, "bootindex"); - -#undef CHECK - - g_strfreev(params); - params = NULL; - - /* present but empty */ - if ((nparams = qemuMonitorGetCommandLineOptionParameters(qemuMonitorTestGetMonitor(test), - "acpi", - ¶ms, - &found)) < 0) - goto cleanup; - - if (nparams != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "nparams was %d, expected 0", nparams); - goto cleanup; - } - if (!found) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - "found was false, expected true"); - goto cleanup; - } - if (params && params[0]) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - "unexpected array contents"); - goto cleanup; - } - - g_strfreev(params); - params = NULL; - - /* no such option */ - if ((nparams = qemuMonitorGetCommandLineOptionParameters(qemuMonitorTestGetMonitor(test), - "foobar", - ¶ms, - &found)) < 0) - goto cleanup; - - if (nparams != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "nparams was %d, expected 0", nparams); - goto cleanup; - } - if (found) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - "found was true, expected false"); - goto cleanup; - } - if (params && params[0]) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - "unexpected array contents"); - goto cleanup; - } - - ret = 0; - - cleanup: - g_strfreev(params); - return ret; -} - - struct qemuMonitorJSONTestAttachChardevData { qemuMonitorTestPtr test; virDomainChrSourceDefPtr chr; @@ -3256,7 +3140,6 @@ mymain(void) DO_TEST(GetCPUDefinitions); DO_TEST(GetCommands); DO_TEST(GetTPMModels); - DO_TEST(GetCommandLineOptionParameters); if (qemuMonitorJSONTestAttachChardev(driver.xmlopt, qapiData.schema) < 0) ret = -1; DO_TEST(DetachChardev); -- 2.28.0
On 11/30/20 6:25 PM, Peter Krempa wrote:
Shuffle around where we extract the data, so that we don't have to call the monitor multiple times which necessitates caching.
Peter Krempa (3): qemu: monitor: Implement new handlers for 'query-command-line-options' virQEMUCapsProbeQMPCommandLine: Rewrite using qemuMonitorGetCommandLineOptions qemuMonitorGetCommandLineOptionParameters: remove the unused function and helpers
src/qemu/qemu_capabilities.c | 34 +++++---- src/qemu/qemu_monitor.c | 34 +-------- src/qemu/qemu_monitor.h | 9 +-- src/qemu/qemu_monitor_json.c | 138 +++++++++-------------------------- src/qemu/qemu_monitor_json.h | 6 +- tests/qemumonitorjsontest.c | 117 ----------------------------- 6 files changed, 58 insertions(+), 280 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Privoznik -
Peter Krempa