Re: [libvirt] [PATCH v5 08/36] qemu_process: All ProcessQMP errors are fatal
On Sun, Dec 02, 2018 at 23:10:02 -0600, Chris Venteicher wrote:
> In the past capabilities could be determined in other ways if QMP
> messaging didn't succeed so a non-fatal error case was included in the
> capabilities and QMP Process code.
>
> For a while now, QMP capabilities failure has been a fatal case.
>
> This patch makes QMP process failures return as a fatal error in all
> cases consistent with 1) all failures actually being fatal in
> QMP capabilities code and 2) the QMP process code being made generic.
>
> The process changes impact the capabilities code because non-fatal
> return codes are no longer returned.
>
> The rest of the QMP associated capabilities code is updated to make all
> errors fatal for consistency.
>
> The process changes also force a change in QMP process stderr reporting
> because the previous triggers for stderr reporting are no longer passed
> through the function interfaces.
>
> As best I can tell the only case where QMP process stderr should be
> explicitly reported is when the QMP process exits with a non-zero
> status.
>
> Error reporting is moved to virQEMUCapsInitQMP and the QMP process
> stderr is reported only when the QMP process status code is non-zero
> (and is only reported for the primary QMP query not the secondary TCG
> specific QMP query.)
Looks like the patch is unnecessarily doing more things at once...
>
> Signed-off-by: Chris Venteicher
> ---
> src/qemu/qemu_capabilities.c | 83
> src/qemu/qemu_process.c | 24 ---
> src/qemu/qemu_process.h | 1 +
> 3 files changed, 45 insertions(+), 63 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index bed9ca26a1..1efec85b57 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
...
> @@ -4234,25 +4231,47 @@ virQEMUCapsInitQMPMonitorTCG(virQEMUCapsPtr qemuCaps
> ATTRIBUTE_UNUSED,
> }
>
>
> +#define MESSAGE_ID_CAPS_PROBE_FAILURE "8ae2f3fb-2dbe-498e-8fbd-012d40afa361"
> +
> +static void
> +virQEMUCapsLogProbeFailure(const char *binary)
> +{
> +virLogMetadata meta[] = {
> +{ .key = "MESSAGE_ID", .s = MESSAGE_ID_CAPS_PROBE_FAILURE, .iv = 0 },
> +{ .key = "LIBVIRT_QEMU_BINARY", .s = binary, .iv = 0 },
> +{ .key = NULL },
> +};
> +
> +virLogMessage(,
> + VIR_LOG_WARN,
> + __FILE__, __LINE__, __func__,
> + meta,
> + _("Failed to probe capabilities for %s: %s"),
> + binary, virGetLastErrorMessage());
> +}
> +
> +
I agree calling virQEMUCapsLogProbeFailure from inside
virQEMUCapsInitQMP makes a bit more sense then doing it in the caller,
but it should be done separately. The change has nothing to do with
the purpose of this patch.
> static int
> virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
> const char *libDir,
> uid_t runUid,
> - gid_t runGid,
> - char **qmperr)
> + gid_t runGid)
This could be moved to the "qemu_process: Persist stderr in
qemuProcessQmp struct" patch. There should be no reason for splitting
a single logical change in two patches.
> {
> qemuProcessQmpPtr proc = NULL;
> qemuProcessQmpPtr procTCG = NULL;
> +char *qmperr = NULL;
> int ret = -1;
> -int rc;
>
> if (!(proc = qemuProcessQmpNew(qemuCaps->binary, libDir,
> - runUid, runGid, qmperr, false)))
> + runUid, runGid, , false)))
> goto cleanup;
>
> -if ((rc = qemuProcessQmpRun(proc)) != 0) {
> -if (rc == 1)
> -ret = 0;
> +if (qemuProcessQmpRun(proc) < 0) {
> +if (proc->status != 0)
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to probe QEMU binary with QMP: %s"),
> + qmperr ? qmperr : _("uknown error"));
[*]
> +
> goto cleanup;
> }
>
> @@ -4270,11 +4289,8 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
> procTCG = qemuProcessQmpNew(qemuCaps->binary, libDir,
> runUid, runGid, NULL, true);
>
> -if ((rc = qemuProcessQmpRun(procTCG)) != 0) {
> -if (rc == 1)
> -ret = 0;
> +if (qemuProcessQmpRun(procTCG) < 0)
> goto cleanup;
This would return -1 without setting any error. The virReportError [*]
from the previous hunk would need to be repeated here. And doing so
would indicate this is a wrong place for reporting the error. The
qemuProcessQmpRun function returned -1 and thus it should have called
virReportError itself.
And as I suggested in my review for the previous version, there's no
need to have two almost identical copies of the same code in
virQEMUCapsInitQMP. Just separate the code into a new function which
will then be called twice in a new patch put before this
[libvirt] [PATCH v5 08/36] qemu_process: All ProcessQMP errors are fatal
In the past capabilities could be determined in other ways if QMP
messaging didn't succeed so a non-fatal error case was included in the
capabilities and QMP Process code.
For a while now, QMP capabilities failure has been a fatal case.
This patch makes QMP process failures return as a fatal error in all
cases consistent with 1) all failures actually being fatal in
QMP capabilities code and 2) the QMP process code being made generic.
The process changes impact the capabilities code because non-fatal
return codes are no longer returned.
The rest of the QMP associated capabilities code is updated to make all
errors fatal for consistency.
The process changes also force a change in QMP process stderr reporting
because the previous triggers for stderr reporting are no longer passed
through the function interfaces.
As best I can tell the only case where QMP process stderr should be
explicitly reported is when the QMP process exits with a non-zero
status.
Error reporting is moved to virQEMUCapsInitQMP and the QMP process
stderr is reported only when the QMP process status code is non-zero
(and is only reported for the primary QMP query not the secondary TCG
specific QMP query.)
Signed-off-by: Chris Venteicher
---
src/qemu/qemu_capabilities.c | 83
src/qemu/qemu_process.c | 24 ---
src/qemu/qemu_process.h | 1 +
3 files changed, 45 insertions(+), 63 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index bed9ca26a1..1efec85b57 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -4038,7 +4038,6 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
if (qemuMonitorSetCapabilities(mon) < 0) {
VIR_DEBUG("Failed to set monitor capabilities %s",
virGetLastErrorMessage());
-ret = 0;
goto cleanup;
}
@@ -4047,7 +4046,6 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
) < 0) {
VIR_DEBUG("Failed to query monitor version %s",
virGetLastErrorMessage());
-ret = 0;
goto cleanup;
}
@@ -4218,7 +4216,6 @@ virQEMUCapsInitQMPMonitorTCG(virQEMUCapsPtr qemuCaps
ATTRIBUTE_UNUSED,
if (qemuMonitorSetCapabilities(mon) < 0) {
VIR_DEBUG("Failed to set monitor capabilities %s",
virGetLastErrorMessage());
-ret = 0;
goto cleanup;
}
@@ -4234,25 +4231,47 @@ virQEMUCapsInitQMPMonitorTCG(virQEMUCapsPtr qemuCaps
ATTRIBUTE_UNUSED,
}
+#define MESSAGE_ID_CAPS_PROBE_FAILURE "8ae2f3fb-2dbe-498e-8fbd-012d40afa361"
+
+static void
+virQEMUCapsLogProbeFailure(const char *binary)
+{
+virLogMetadata meta[] = {
+{ .key = "MESSAGE_ID", .s = MESSAGE_ID_CAPS_PROBE_FAILURE, .iv = 0 },
+{ .key = "LIBVIRT_QEMU_BINARY", .s = binary, .iv = 0 },
+{ .key = NULL },
+};
+
+virLogMessage(,
+ VIR_LOG_WARN,
+ __FILE__, __LINE__, __func__,
+ meta,
+ _("Failed to probe capabilities for %s: %s"),
+ binary, virGetLastErrorMessage());
+}
+
+
static int
virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
const char *libDir,
uid_t runUid,
- gid_t runGid,
- char **qmperr)
+ gid_t runGid)
{
qemuProcessQmpPtr proc = NULL;
qemuProcessQmpPtr procTCG = NULL;
+char *qmperr = NULL;
int ret = -1;
-int rc;
if (!(proc = qemuProcessQmpNew(qemuCaps->binary, libDir,
- runUid, runGid, qmperr, false)))
+ runUid, runGid, , false)))
goto cleanup;
-if ((rc = qemuProcessQmpRun(proc)) != 0) {
-if (rc == 1)
-ret = 0;
+if (qemuProcessQmpRun(proc) < 0) {
+if (proc->status != 0)
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to probe QEMU binary with QMP: %s"),
+ qmperr ? qmperr : _("uknown error"));
+
goto cleanup;
}
@@ -4270,11 +4289,8 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
procTCG = qemuProcessQmpNew(qemuCaps->binary, libDir,
runUid, runGid, NULL, true);
-if ((rc = qemuProcessQmpRun(procTCG)) != 0) {
-if (rc == 1)
-ret = 0;
+if (qemuProcessQmpRun(procTCG) < 0)
goto cleanup;
-}
if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, procTCG->mon) < 0)
goto cleanup;
@@ -4283,34 +4299,19 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
ret = 0;
cleanup:
+if (ret < 0)
+virQEMUCapsLogProbeFailure(qemuCaps->binary);
+
qemuProcessQmpStop(proc);
qemuProcessQmpStop(procTCG);
qemuProcessQmpFree(proc);
qemuProcessQmpFree(procTCG);
+VIR_FREE(qmperr);
+
return ret;
}
