[PATCH 0/5] qemu: Don't log spurious errors on qemuMonitorDelObject
qemuMonitorDelObject is often used in cleanup cases so we need to control whether to log errors. First patch actually prevents one of the spurious calls in cases we know it would be pointless. Peter Krempa (5): qemuDomainChangeEjectableMedia: Don't always remove managed PR daemon qemuMonitorJSON(Add|Del)Object: Refactor cleanup qemuMonitorJSONCheckError: Use g_autofree qemuMonitorJSONCheckError: Allow suppressing of error reporting qemu: Suppress error reporting from qemuMonitorDelObject in cleanup paths src/qemu/qemu_block.c | 10 ++--- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 34 +++++++++------- src/qemu/qemu_monitor.c | 5 ++- src/qemu/qemu_monitor.h | 3 +- src/qemu/qemu_monitor_json.c | 77 ++++++++++++++++++------------------ src/qemu/qemu_monitor_json.h | 3 +- 7 files changed, 71 insertions(+), 63 deletions(-) -- 2.24.1
When changing media we'd attempt to remove the managed pr daemon even if neither of the images involved in the media change used it. This caused libvirtd to log a spurious error: 2020-03-18 01:41:19.832+0000: 643207: error : qemuMonitorJSONCheckError:412 : internal error: unable to execute QEMU command 'object-del': object 'pr-helper0' not found With this patch we completely avoid calling the deletion code. https://bugzilla.redhat.com/show_bug.cgi?id=1814486 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 47069be900..1cab12fb2b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -590,6 +590,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, virStorageSourcePtr oldsrc = disk->src; qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); bool sharedAdded = false; + bool managedpr = virStorageSourceChainHasManagedPR(oldsrc) || + virStorageSourceChainHasManagedPR(newsrc); int ret = -1; int rc; @@ -653,7 +655,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, } /* remove PR manager object if unneeded */ - ignore_value(qemuHotplugRemoveManagedPR(driver, vm, QEMU_ASYNC_JOB_NONE)); + if (managedpr) + ignore_value(qemuHotplugRemoveManagedPR(driver, vm, QEMU_ASYNC_JOB_NONE)); /* revert old image do the disk definition */ if (oldsrc) -- 2.24.1
Use 'g_autoptr' and remove the cleanup label and ret variable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 42 +++++++++++++----------------------- 1 file changed, 15 insertions(+), 27 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 8ac8291d0a..00d7760a05 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4418,51 +4418,39 @@ int qemuMonitorJSONAddObject(qemuMonitorPtr mon, virJSONValuePtr props) { - int ret = -1; - virJSONValuePtr cmd; - virJSONValuePtr reply = NULL; + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; if (!(cmd = qemuMonitorJSONMakeCommandInternal("object-add", props))) - goto cleanup; + return -1; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) - goto cleanup; + return -1; if (qemuMonitorJSONCheckError(cmd, reply) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - virJSONValueFree(cmd); - virJSONValueFree(reply); - return ret; + return 0; } -int qemuMonitorJSONDelObject(qemuMonitorPtr mon, - const char *objalias) +int +qemuMonitorJSONDelObject(qemuMonitorPtr mon, + const char *objalias) { - int ret = -1; - virJSONValuePtr cmd; - virJSONValuePtr reply = NULL; + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; - cmd = qemuMonitorJSONMakeCommand("object-del", - "s:id", objalias, - NULL); - if (!cmd) + if (!(cmd = qemuMonitorJSONMakeCommand("object-del", "s:id", objalias, NULL))) return -1; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) - goto cleanup; + return -1; if (qemuMonitorJSONCheckError(cmd, reply) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - virJSONValueFree(cmd); - virJSONValueFree(reply); - return ret; + return 0; } -- 2.24.1
Eliminate cleanup code by using g_autofree. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 00d7760a05..31eb01006c 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -393,8 +393,8 @@ qemuMonitorJSONCheckError(virJSONValuePtr cmd, { if (virJSONValueObjectHasKey(reply, "error")) { virJSONValuePtr error = virJSONValueObjectGet(reply, "error"); - char *cmdstr = virJSONValueToString(cmd, false); - char *replystr = virJSONValueToString(reply, false); + g_autofree char *cmdstr = virJSONValueToString(cmd, false); + g_autofree char *replystr = virJSONValueToString(reply, false); /* Log the full JSON formatted command & error */ VIR_DEBUG("unable to execute QEMU command %s: %s", @@ -411,20 +411,16 @@ qemuMonitorJSONCheckError(virJSONValuePtr cmd, qemuMonitorJSONCommandName(cmd), qemuMonitorJSONStringifyError(error)); - VIR_FREE(cmdstr); - VIR_FREE(replystr); return -1; } else if (!virJSONValueObjectHasKey(reply, "return")) { - char *cmdstr = virJSONValueToString(cmd, false); - char *replystr = virJSONValueToString(reply, false); + g_autofree char *cmdstr = virJSONValueToString(cmd, false); + g_autofree char *replystr = virJSONValueToString(reply, false); VIR_DEBUG("Neither 'return' nor 'error' is set in the JSON reply %s: %s", NULLSTR(cmdstr), NULLSTR(replystr)); virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to execute QEMU command '%s'"), qemuMonitorJSONCommandName(cmd)); - VIR_FREE(cmdstr); - VIR_FREE(replystr); return -1; } return 0; -- 2.24.1
In some cases we'll need to check whether there was an error but avoid reporting an actual libvirt error. Rename qemuMonitorJSONCheckError to qemuMonitorJSONCheckErrorFull with a new flag to suppress the error reporting and add a wrapper with the original name so that callers don't need to be fixed. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 31eb01006c..c18cef5c1a 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -388,8 +388,9 @@ qemuMonitorJSONCommandName(virJSONValuePtr cmd) } static int -qemuMonitorJSONCheckError(virJSONValuePtr cmd, - virJSONValuePtr reply) +qemuMonitorJSONCheckErrorFull(virJSONValuePtr cmd, + virJSONValuePtr reply, + bool report) { if (virJSONValueObjectHasKey(reply, "error")) { virJSONValuePtr error = virJSONValueObjectGet(reply, "error"); @@ -400,6 +401,9 @@ qemuMonitorJSONCheckError(virJSONValuePtr cmd, VIR_DEBUG("unable to execute QEMU command %s: %s", NULLSTR(cmdstr), NULLSTR(replystr)); + if (!report) + return -1; + /* Only send the user the command name + friendly error */ if (!error) virReportError(VIR_ERR_INTERNAL_ERROR, @@ -418,6 +422,10 @@ qemuMonitorJSONCheckError(virJSONValuePtr cmd, VIR_DEBUG("Neither 'return' nor 'error' is set in the JSON reply %s: %s", NULLSTR(cmdstr), NULLSTR(replystr)); + + if (!report) + return -1; + virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to execute QEMU command '%s'"), qemuMonitorJSONCommandName(cmd)); @@ -427,6 +435,14 @@ qemuMonitorJSONCheckError(virJSONValuePtr cmd, } +static int +qemuMonitorJSONCheckError(virJSONValuePtr cmd, + virJSONValuePtr reply) +{ + return qemuMonitorJSONCheckErrorFull(cmd, reply, true); +} + + static int qemuMonitorJSONCheckReply(virJSONValuePtr cmd, virJSONValuePtr reply, -- 2.24.1
Many calls of qemuMonitorDelObject don't actually check the return value or report the error from the object deletion itself since they are on cleanup paths. In some cases this can lead to reporting of spurious errors e.g. when qemuMonitorDelObject is used to clean up a possibly pre-existing objects. Add a new argument for qemuMonitorDelObject which controls whether the internals report errors from qemu and fix all callers accordingly. Note that some of the cases on device unplug which check the error code don't in fact propagate the error to the user, but in this case it is important to add the log entry anyways for tracing that the device deletion failed. https://bugzilla.redhat.com/show_bug.cgi?id=1784040 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 10 +++++----- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 29 +++++++++++++++-------------- src/qemu/qemu_monitor.c | 5 +++-- src/qemu/qemu_monitor.h | 3 ++- src/qemu/qemu_monitor_json.c | 5 +++-- src/qemu/qemu_monitor_json.h | 3 ++- 7 files changed, 31 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index f95ebb6fa7..4ed17b6df3 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1732,19 +1732,19 @@ qemuBlockStorageSourceAttachRollback(qemuMonitorPtr mon, ignore_value(qemuMonitorBlockdevDel(mon, data->storageNodeName)); if (data->prmgrAlias) - ignore_value(qemuMonitorDelObject(mon, data->prmgrAlias)); + ignore_value(qemuMonitorDelObject(mon, data->prmgrAlias, false)); if (data->authsecretAlias) - ignore_value(qemuMonitorDelObject(mon, data->authsecretAlias)); + ignore_value(qemuMonitorDelObject(mon, data->authsecretAlias, false)); if (data->encryptsecretAlias) - ignore_value(qemuMonitorDelObject(mon, data->encryptsecretAlias)); + ignore_value(qemuMonitorDelObject(mon, data->encryptsecretAlias, false)); if (data->httpcookiesecretAlias) - ignore_value(qemuMonitorDelObject(mon, data->httpcookiesecretAlias)); + ignore_value(qemuMonitorDelObject(mon, data->httpcookiesecretAlias, false)); if (data->tlsAlias) - ignore_value(qemuMonitorDelObject(mon, data->tlsAlias)); + ignore_value(qemuMonitorDelObject(mon, data->tlsAlias, false)); virErrorRestore(&orig_err); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b62947fb4a..8a2cd35fa7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6047,7 +6047,7 @@ qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); - rc = qemuMonitorDelObject(priv->mon, alias); + rc = qemuMonitorDelObject(priv->mon, alias, true); exp_niothreads--; if (rc < 0) goto exit_monitor; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1cab12fb2b..bbe721c02c 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -390,7 +390,8 @@ qemuHotplugRemoveManagedPR(virQEMUDriverPtr driver, if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) goto cleanup; - ignore_value(qemuMonitorDelObject(priv->mon, qemuDomainGetManagedPRAlias())); + ignore_value(qemuMonitorDelObject(priv->mon, qemuDomainGetManagedPRAlias(), + false)); if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; @@ -471,7 +472,7 @@ qemuDomainDetachDBusVMState(virQEMUDriverPtr driver, qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) return -1; - ret = qemuMonitorDelObject(priv->mon, alias); + ret = qemuMonitorDelObject(priv->mon, alias, true); if (qemuDomainObjExitMonitor(driver, vm) < 0) return -1; @@ -1685,10 +1686,10 @@ qemuDomainDelTLSObjects(virQEMUDriverPtr driver, goto cleanup; if (tlsAlias) - ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); + ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias, false)); if (secAlias) - ignore_value(qemuMonitorDelObject(priv->mon, secAlias)); + ignore_value(qemuMonitorDelObject(priv->mon, secAlias, false)); ignore_value(qemuDomainObjExitMonitor(driver, vm)); @@ -1850,9 +1851,9 @@ qemuDomainDelChardevTLSObjects(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); - ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); + ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias, false)); if (secAlias) - ignore_value(qemuMonitorDelObject(priv->mon, secAlias)); + ignore_value(qemuMonitorDelObject(priv->mon, secAlias, false)); if (qemuDomainObjExitMonitor(driver, vm) < 0) return -1; @@ -2307,7 +2308,7 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, exit_monitor: virErrorPreserveLast(&orig_err); if (objAlias) - ignore_value(qemuMonitorDelObject(priv->mon, objAlias)); + ignore_value(qemuMonitorDelObject(priv->mon, objAlias, false)); if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD && chardevAdded) ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); if (qemuDomainObjExitMonitor(driver, vm) < 0) @@ -2443,7 +2444,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, exit_monitor: virErrorPreserveLast(&orig_err); if (objAdded) - ignore_value(qemuMonitorDelObject(priv->mon, objalias)); + ignore_value(qemuMonitorDelObject(priv->mon, objalias, false)); if (qemuDomainObjExitMonitor(driver, vm) < 0) mem = NULL; @@ -2665,7 +2666,7 @@ qemuDomainAttachHostSCSIDevice(virQEMUDriverPtr driver, drvstr, devstr); } if (secobjAlias) - ignore_value(qemuMonitorDelObject(priv->mon, secobjAlias)); + ignore_value(qemuMonitorDelObject(priv->mon, secobjAlias, false)); ignore_value(qemuDomainObjExitMonitor(driver, vm)); virErrorRestore(&orig_err); @@ -3041,7 +3042,7 @@ qemuDomainAttachShmemDevice(virQEMUDriverPtr driver, if (shmem->server.enabled) ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); else - ignore_value(qemuMonitorDelObject(priv->mon, memAlias)); + ignore_value(qemuMonitorDelObject(priv->mon, memAlias, false)); } if (qemuDomainObjExitMonitor(driver, vm) < 0) @@ -4367,7 +4368,7 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver, backendAlias = g_strdup_printf("mem%s", mem->info.alias); qemuDomainObjEnterMonitor(driver, vm); - rc = qemuMonitorDelObject(priv->mon, backendAlias); + rc = qemuMonitorDelObject(priv->mon, backendAlias, true); if (qemuDomainObjExitMonitor(driver, vm) < 0) rc = -1; @@ -4483,7 +4484,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, /* If it fails, then so be it - it was a best shot */ if (objAlias) - ignore_value(qemuMonitorDelObject(priv->mon, objAlias)); + ignore_value(qemuMonitorDelObject(priv->mon, objAlias, false)); if (qemuDomainObjExitMonitor(driver, vm) < 0) return -1; @@ -4741,7 +4742,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); if (rc == 0 && - qemuMonitorDelObject(priv->mon, objAlias) < 0) + qemuMonitorDelObject(priv->mon, objAlias, true) < 0) rc = -1; if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD && @@ -4802,7 +4803,7 @@ qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver, if (shmem->server.enabled) rc = qemuMonitorDetachCharDev(priv->mon, charAlias); else - rc = qemuMonitorDelObject(priv->mon, memAlias); + rc = qemuMonitorDelObject(priv->mon, memAlias, true); if (qemuDomainObjExitMonitor(driver, vm) < 0) return -1; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 4a5bd1d4ab..3ac78016e2 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2966,13 +2966,14 @@ qemuMonitorAddObject(qemuMonitorPtr mon, int qemuMonitorDelObject(qemuMonitorPtr mon, - const char *objalias) + const char *objalias, + bool report_error) { VIR_DEBUG("objalias=%s", objalias); QEMU_CHECK_MONITOR(mon); - return qemuMonitorJSONDelObject(mon, objalias); + return qemuMonitorJSONDelObject(mon, objalias, report_error); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 2437375327..e2cc12bd0f 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -931,7 +931,8 @@ int qemuMonitorAddObject(qemuMonitorPtr mon, ATTRIBUTE_NONNULL(2); int qemuMonitorDelObject(qemuMonitorPtr mon, - const char *objalias); + const char *objalias, + bool report_error); int qemuMonitorAddDrive(qemuMonitorPtr mon, const char *drivestr); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index c18cef5c1a..a18ab477ee 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4448,7 +4448,8 @@ qemuMonitorJSONAddObject(qemuMonitorPtr mon, int qemuMonitorJSONDelObject(qemuMonitorPtr mon, - const char *objalias) + const char *objalias, + bool report_error) { g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; @@ -4459,7 +4460,7 @@ qemuMonitorJSONDelObject(qemuMonitorPtr mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) return -1; - if (qemuMonitorJSONCheckError(cmd, reply) < 0) + if (qemuMonitorJSONCheckErrorFull(cmd, reply, report_error) < 0) return -1; return 0; diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 5b3bb295eb..46dc22f1a4 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -237,7 +237,8 @@ int qemuMonitorJSONAddObject(qemuMonitorPtr mon, virJSONValuePtr props); int qemuMonitorJSONDelObject(qemuMonitorPtr mon, - const char *objalias); + const char *objalias, + bool report_error); int qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr *actions) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -- 2.24.1
On 18. 3. 2020 12:40, Peter Krempa wrote:
qemuMonitorDelObject is often used in cleanup cases so we need to control whether to log errors.
First patch actually prevents one of the spurious calls in cases we know it would be pointless.
Peter Krempa (5): qemuDomainChangeEjectableMedia: Don't always remove managed PR daemon qemuMonitorJSON(Add|Del)Object: Refactor cleanup qemuMonitorJSONCheckError: Use g_autofree qemuMonitorJSONCheckError: Allow suppressing of error reporting qemu: Suppress error reporting from qemuMonitorDelObject in cleanup paths
src/qemu/qemu_block.c | 10 ++--- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 34 +++++++++------- src/qemu/qemu_monitor.c | 5 ++- src/qemu/qemu_monitor.h | 3 +- src/qemu/qemu_monitor_json.c | 77 ++++++++++++++++++------------------ src/qemu/qemu_monitor_json.h | 3 +- 7 files changed, 71 insertions(+), 63 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Prívozník -
Peter Krempa