[libvirt] [PATCH v2] qemuDomainObjBeginJobInternal: Report agent job in error message
If a thread is unable to acquire a job (e.g. because of timeout)
an error is reported and the error message contains reference to
the other thread holding the job. Well, the error message should
report agent job too as it is yet another source of possible
failure.
Signed-off-by: Michal Privoznik
---
diff to v1:
- Expand messages to cover all @blocker and @agentBlocker possibilities
(as Jira requested in review)
src/qemu/qemu_domain.c | 46 ++
1 file changed, 38 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 95c3e3a8aa..3410774781 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6420,6 +6420,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
bool async = job == QEMU_JOB_ASYNC;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
const char *blocker = NULL;
+const char *agentBlocker = NULL;
int ret = -1;
unsigned long long duration = 0;
unsigned long long agentDuration = 0;
@@ -6549,16 +6550,32 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
priv->job.apiFlags,
duration / 1000, agentDuration / 1000, asyncDuration / 1000);
-if (nested || qemuDomainNestedJobAllowed(priv, job))
-blocker = priv->job.ownerAPI;
-else
-blocker = priv->job.asyncOwnerAPI;
+if (job) {
+if (nested || qemuDomainNestedJobAllowed(priv, job))
+blocker = priv->job.ownerAPI;
+else
+blocker = priv->job.asyncOwnerAPI;
+}
+
+if (agentJob)
+agentBlocker = priv->job.agentOwnerAPI;
if (errno == ETIMEDOUT) {
-if (blocker) {
+if (blocker && agentBlocker) {
virReportError(VIR_ERR_OPERATION_TIMEOUT,
- _("cannot acquire state change lock (held by %s)"),
+ _("cannot acquire state change "
+ "lock (held by monitor=%s agent=%s)"),
+ blocker, agentBlocker);
+} else if (blocker) {
+virReportError(VIR_ERR_OPERATION_TIMEOUT,
+ _("cannot acquire state change "
+ "lock (held by monitor=%s)"),
blocker);
+} else if (agentBlocker) {
+virReportError(VIR_ERR_OPERATION_TIMEOUT,
+ _("cannot acquire state change "
+ "lock (held by agent=%s)"),
+ agentBlocker);
} else {
virReportError(VIR_ERR_OPERATION_TIMEOUT, "%s",
_("cannot acquire state change lock"));
@@ -6566,11 +6583,24 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
ret = -2;
} else if (cfg->maxQueuedJobs &&
priv->jobs_queued > cfg->maxQueuedJobs) {
-if (blocker) {
+if (blocker && agentBlocker) {
virReportError(VIR_ERR_OPERATION_FAILED,
- _("cannot acquire state change lock (held by %s) "
+ _("cannot acquire state change "
+ "lock (held by monitor=%s agent=%s) "
+ "due to max_queued limit"),
+ blocker, agentBlocker);
+} else if (blocker) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+ _("cannot acquire state change "
+ "lock (held by monitor=%s) "
"due to max_queued limit"),
blocker);
+} else if (agentBlocker) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+ _("cannot acquire state change "
+ "lock (held by agent=%s) "
+ "due to max_queued limit"),
+ agentBlocker);
} else {
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("cannot acquire state change lock "
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemuDomainObjBeginJobInternal: Report agent job in error message
On 06/22/2018 10:07 AM, Jiri Denemark wrote:
> On Wed, Jun 20, 2018 at 14:32:04 +0200, Michal Privoznik wrote:
>> If a thread is unable to acquire a job (e.g. because of timeout)
>> an error is reported and the error message contains reference to
>> the other thread holding the job. Well, the error message should
>> report agent job too as it is yet another source of possible
>> failure.
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/qemu/qemu_domain.c | 26 --
>> 1 file changed, 16 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 827597d5f3..4331b95917 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -6420,6 +6420,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
>> bool async = job == QEMU_JOB_ASYNC;
>> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>> const char *blocker = NULL;
>> +const char *agentBlocker = NULL;
>> int ret = -1;
>> unsigned long long duration = 0;
>> unsigned long long agentDuration = 0;
>> @@ -6549,16 +6550,21 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr
>> driver,
>> priv->job.apiFlags,
>> duration / 1000, agentDuration / 1000, asyncDuration / 1000);
>>
>> -if (nested || qemuDomainNestedJobAllowed(priv, job))
>> -blocker = priv->job.ownerAPI;
>> -else
>> -blocker = priv->job.asyncOwnerAPI;
>> +if (job) {
>> +if (nested || qemuDomainNestedJobAllowed(priv, job))
>> +blocker = priv->job.ownerAPI;
>> +else
>> +blocker = priv->job.asyncOwnerAPI;
>> +}
>> +
>> +if (agentJob)
>> +agentBlocker = priv->job.agentOwnerAPI;
>>
>> if (errno == ETIMEDOUT) {
>> -if (blocker) {
>> +if (blocker || agentBlocker) {
>> virReportError(VIR_ERR_OPERATION_TIMEOUT,
>> - _("cannot acquire state change lock (held by
>> %s)"),
>> - blocker);
>> + _("cannot acquire state change lock (held by %s
>> %s)"),
>> + NULLSTR(blocker), NULLSTR(agentBlocker));
>
> Since this is an error message reported to the user I think we should
> make it a little bit more user friendly. It would be nice to distinguish
> state change lock and agent lock and only print the relevant blocker
> (rather than both when one of them is NULL). And when both blockers are
> reported, we should separate them better, e.g., "%s and %s".
I thought about it too, but then decided to take the easy way out
because this would end up being a spaghetti code in my sight. But okay,
I will rework this to:
if (blocker && agentBlocker) {
...
} else if (blocker) {
...
} else if (agentBlocker) {
...
} else {
...
}
where all four bodies are merely the same (only the error message would
change - hence the spaghetti code).
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: ensure FDs passed to QEMU for chardevs have correct SELinux labels
On 06/22/2018 05:56 AM, Daniel P. Berrangé wrote:
> The UNIX socket FDs were we passing to QEMU inherited a label based on
> libvirtd's context. QEMU is thus denied ability to access the UNIX
> socket. We need to use the security manager to change our current
> context temporarily when creating the UNIX socket FD.
It seems like this could be more efficient if we only set the label
once, then cleared it once (for the entire domain), rather than going
through all that trouble for each socket. But I suppose that would
probably cause problems if there were any other sockets created in the
interim that were *not* destined for use with qemu (and likely other
problems in the opposite direction in other places).
This does fix the failure to start domains with selinux enabled that I
was experiencing, which trumps any whiny perception of inefficiency.
>
> Signed-off-by: Daniel P. Berrangé
Reviewed-by: Laine Stump
> ---
> src/qemu/qemu_command.c | 86 -
> src/qemu/qemu_command.h | 1 +
> src/qemu/qemu_process.c | 2 +
> 3 files changed, 63 insertions(+), 26 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 1ffcb5b1ae..146f671663 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4931,6 +4931,7 @@ qemuOpenChrChardevUNIXSocket(const
> virDomainChrSourceDef *dev)
> * host side of the character device */
> static char *
> qemuBuildChrChardevStr(virLogManagerPtr logManager,
> + virSecurityManagerPtr secManager,
> virCommandPtr cmd,
> virQEMUDriverConfigPtr cfg,
> const virDomainDef *def,
> @@ -5065,7 +5066,13 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
>
> case VIR_DOMAIN_CHR_TYPE_UNIX:
> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS)) {
> +if (qemuSecuritySetSocketLabel(secManager, (virDomainDefPtr)def)
> < 0)
> +goto cleanup;
> int fd = qemuOpenChrChardevUNIXSocket(dev);
> +if (qemuSecurityClearSocketLabel(secManager,
> (virDomainDefPtr)def) < 0) {
> +VIR_FORCE_CLOSE(fd);
> +goto cleanup;
> +}
> if (fd < 0)
> goto cleanup;
>
> @@ -5404,6 +5411,7 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
>
> static int
> qemuBuildMonitorCommandLine(virLogManagerPtr logManager,
> +virSecurityManagerPtr secManager,
> virCommandPtr cmd,
> virQEMUDriverConfigPtr cfg,
> virDomainDefPtr def,
> @@ -5414,7 +5422,8 @@ qemuBuildMonitorCommandLine(virLogManagerPtr logManager,
> if (!priv->monConfig)
> return 0;
>
> -if (!(chrdev = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
> +if (!(chrdev = qemuBuildChrChardevStr(logManager, secManager,
> + cmd, cfg, def,
>priv->monConfig, "monitor",
>priv->qemuCaps, true,
>priv->chardevStdioLogd)))
> @@ -5533,6 +5542,7 @@ qemuBuildSclpDevStr(virDomainChrDefPtr dev)
>
> static int
> qemuBuildRNGBackendChrdevStr(virLogManagerPtr logManager,
> + virSecurityManagerPtr secManager,
> virCommandPtr cmd,
> virQEMUDriverConfigPtr cfg,
> const virDomainDef *def,
> @@ -5550,7 +5560,8 @@ qemuBuildRNGBackendChrdevStr(virLogManagerPtr
> logManager,
> return 0;
>
> case VIR_DOMAIN_RNG_BACKEND_EGD:
> -if (!(*chr = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
> +if (!(*chr = qemuBuildChrChardevStr(logManager, secManager,
> +cmd, cfg, def,
> rng->source.chardev,
> rng->info.alias, qemuCaps, true,
> chardevStdioLogd)))
> @@ -5680,6 +5691,7 @@ qemuBuildRNGDevStr(const virDomainDef *def,
>
> static int
> qemuBuildRNGCommandLine(virLogManagerPtr logManager,
> +virSecurityManagerPtr secManager,
> virCommandPtr cmd,
> virQEMUDriverConfigPtr cfg,
> const virDomainDef *def,
> @@ -5702,7 +5714,7 @@ qemuBuildRNGCommandLine(virLogManagerPtr logManager,
> }
>
> /* possibly add character device for backend */
> -if (qemuBuildRNGBackendChrdevStr(logManager, cmd, cfg, def,
> +if (qemuBuildRNGBackendChrdevStr(logManager, secManager, cmd, cfg,
> def,
> rng, qemuCaps, &tmp,
> chardevStdioLogd) < 0)
>
