[libvirt] [PATCH 05/10] qemu: agent: straigten up failed agent start case

[John Ferlan]

On 10/04/2016 02:56 AM, Nikolay Shirokovskiy wrote:
> agentError is used for 2 different cases:
> 
> 1. agent monitor is failed to start

Non guest fatal failure in qemuConnectAgent when first trying to connect
to the agent

> 2. io error in agent monitor

I/O error with running agent resulting in qemuProcessHandleAgentError
being called

Most of the above would seemingly be a reason for the removal of the
agentError (which needs to be a separate patch)...

> 
> Actually to check for the first case we don't need the
> flag at all. NULL agent is always error for old qemu
> (QEMU_CAPS_VSERPORT_CHANGE is not supported), while
> for modern qemu it is an error only if channel is in
> connected state.

I suppose this explanation could be added too, but I'd have to see it in
the context of what I've learned so far in a "new" series.

In any case, none of the above text seems to have anything to do with
the change in reporting the errors at startup.

> ---
>  src/qemu/qemu_domain.c  | 37 +++++++++++++++++++++++--------------
>  src/qemu/qemu_process.c |  1 -
>  2 files changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 9b1a32e..cd788c8 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5046,6 +5046,7 @@ qemuDomainAgentAvailable(virDomainObjPtr vm,
>                           bool reportError)
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
> +    virDomainChrDefPtr config;
>  
>      if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_RUNNING) {
>          if (reportError) {
> @@ -5062,22 +5063,30 @@ qemuDomainAgentAvailable(virDomainObjPtr vm,
>          }
>          return false;
>      }
> -    if (!priv->agent) {
> -        if (qemuFindAgentConfig(vm->def)) {
> -            if (reportError) {
> -                virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s",
> -                               _("QEMU guest agent is not connected"));
> -            }
> -            return false;
> -        } else {
> -            if (reportError) {
> -                virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> -                               _("QEMU guest agent is not configured"));
> -            }
> -            return false;
> +
> +    if (priv->agent)
> +        return true;
> +
> +    config = qemuFindAgentConfig(vm->def);
> +
> +    if (!config) {
> +        if (reportError) {
> +            virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> +                           _("QEMU guest agent is not configured"));
>          }

I'm OK through here, but the next two I'm not so sure I agree with.

> +    } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VSERPORT_CHANGE) &&
> +               config->state != VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED) {

What does the manner of startup (either legacy or VSERPORT) have to do
with which error should be reported.
> +        if (reportError) {
> +            virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s",
> +                           _("QEMU guest agent is not connected"));
> +        }
> +    } else if (reportError) {
> +        virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s",
> +                       _("QEMU guest agent is not "
> +                         "available due to an error"));

I know you're setting up for the next set of patches (to remove the
agentError flag), but I'm not sure yet whether using VSERPORT change
configuration is the right way.

Unless of course my assumption from patch 1 review is true... This is
really a startup/shutdown race...

John
>      }
> -    return true;
> +
> +    return false;
>  }
>  
>  
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index d7c9ce3..78d530f 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -267,7 +267,6 @@ qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm)
>   cleanup:
>      if (!priv->agent) {
>          VIR_WARN("Cannot connect to QEMU guest agent for %s", vm->def->name);
> -        priv->agentError = true;

Wait, what, why?  you just added this in the previous patch and I see no
reason for removing it now.

This would need a separate patch and reason.

John

>          virResetLastError();
>      }
>  
>