Re: [PATCH 07/10] qemu: agent: simplify io loop

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 10/04/2016 02:56 AM, Nikolay Shirokovskiy wrote:
> Now we don't need to differentiate error and eof cases in the loop function.
> So let's simplify it radically using goto instead of flags.
> ---
>  src/qemu/qemu_agent.c        | 185 ++++++++++++++++++-------------------------
>  src/qemu/qemu_agent.h        |   2 -
>  src/qemu/qemu_process.c      |  22 +----
>  tests/qemumonitortestutils.c |   1 -
>  4 files changed, 83 insertions(+), 127 deletions(-)
> 

I would think it's understood the genesis of this comes from
qemuMonitorIO (tripped me up in my patch 1 review too, but still the
same result)...

Part of me believes whatever happens here could be considered for
qemuMonitorIO too, but that's perhaps a bigger hurdle to jump.

I'm not sure why we want to make Error and EOF be the same thing.

John

I'm stopping here...

> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index ec8d47e..43d78c9 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -595,8 +595,8 @@ static void
>  qemuAgentIO(int watch, int fd, int events, void *opaque)
>  {
>      qemuAgentPtr mon = opaque;
> -    bool error = false;
> -    bool eof = false;
> +    void (*errorNotify)(qemuAgentPtr, virDomainObjPtr);
> +    virDomainObjPtr vm;
>  
>      virObjectRef(mon);
>      /* lock access to the monitor and protect fd */
> @@ -606,122 +606,95 @@ qemuAgentIO(int watch, int fd, int events, void *opaque)
>  #endif
>  
>      if (mon->fd != fd || mon->watch != watch) {
> -        if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR))
> -            eof = true;
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("event from unexpected fd %d!=%d / watch %d!=%d"),
>                         mon->fd, fd, mon->watch, watch);
> -        error = true;
> -    } else if (mon->lastError.code != VIR_ERR_OK) {
> -        if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR))
> -            eof = true;
> -        error = true;
> -    } else {
> -        if (events & VIR_EVENT_HANDLE_WRITABLE) {
> -            if (mon->connectPending) {
> -                if (qemuAgentIOConnect(mon) < 0)
> -                    error = true;
> -            } else {
> -                if (qemuAgentIOWrite(mon) < 0)
> -                    error = true;
> -            }
> -            events &= ~VIR_EVENT_HANDLE_WRITABLE;
> -        }
> -
> -        if (!error &&
> -            events & VIR_EVENT_HANDLE_READABLE) {
> -            int got = qemuAgentIORead(mon);
> -            events &= ~VIR_EVENT_HANDLE_READABLE;
> -            if (got < 0) {
> -                error = true;
> -            } else if (got == 0) {
> -                eof = true;
> -            } else {
> -                /* Ignore hangup/error events if we read some data, to
> -                 * give time for that data to be consumed */
> -                events = 0;
> -
> -                if (qemuAgentIOProcess(mon) < 0)
> -                    error = true;
> -            }
> -        }
> -
> -        if (!error &&
> -            events & VIR_EVENT_HANDLE_HANGUP) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                           _("End of file from agent monitor"));
> -            eof = true;
> -            events &= ~VIR_EVENT_HANDLE_HANGUP;
> -        }
> -
> -        if (!error && !eof &&
> -            events & VIR_EVENT_HANDLE_ERROR) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                           _("Invalid file descriptor while waiting for monitor"));
> -            eof = true;
> -            events &= ~VIR_EVENT_HANDLE_ERROR;
> -        }
> -        if (!error && events) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR,
> -                           _("Unhandled event %d for monitor fd %d"),
> -                           events, mon->fd);
> -            error = true;
> -        }
> +        goto error;
>      }
>  
> -    if (error || eof) {
> -        if (mon->lastError.code != VIR_ERR_OK) {
> -            /* Already have an error, so clear any new error */
> -            virResetLastError();
> +    if (mon->lastError.code != VIR_ERR_OK)
> +        goto error;
> +
> +    if (events & VIR_EVENT_HANDLE_WRITABLE) {
> +        if (mon->connectPending) {
> +            if (qemuAgentIOConnect(mon) < 0)
> +                goto error;
>          } else {
> -            virErrorPtr err = virGetLastError();
> -            if (!err)
> -                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                               _("Error while processing monitor IO"));
> -            virCopyLastError(&mon->lastError);
> -            virResetLastError();
> +            if (qemuAgentIOWrite(mon) < 0)
> +                goto error;
>          }
> +        events &= ~VIR_EVENT_HANDLE_WRITABLE;
> +    }
>  
> -        VIR_DEBUG("Error on monitor %s", NULLSTR(mon->lastError.message));
> -        /* If IO process resulted in an error & we have a message,
> -         * then wakeup that waiter */
> -        if (mon->msg && !mon->msg->finished) {
> -            mon->msg->finished = 1;
> -            virCondSignal(&mon->notify);
> -        }
> +    if (events & VIR_EVENT_HANDLE_READABLE) {
> +        int got = qemuAgentIORead(mon);
> +
> +        if (got <= 0)
> +            goto error;
> +
> +        if (qemuAgentIOProcess(mon) < 0)
> +            goto error;
> +
> +        /* Ignore hangup/error events if we read some data, to
> +         * give time for that data to be consumed */
> +        events = 0;
> +    }
> +
> +    if (events & VIR_EVENT_HANDLE_HANGUP) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("End of file from agent monitor"));
> +        goto error;
> +    }
> +
> +    if (events & VIR_EVENT_HANDLE_ERROR) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Invalid file descriptor while waiting for monitor"));
> +        goto error;
> +    }
> +
> +    if (events) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Unhandled event %d for monitor fd %d"),
> +                       events, mon->fd);
> +        goto error;
>      }
>  
>      qemuAgentUpdateWatch(mon);
> +    virObjectUnlock(mon);
> +    virObjectUnref(mon);
> +    return;
>  
> -    /* We have to unlock to avoid deadlock against command thread,
> -     * but is this safe ?  I think it is, because the callback
> -     * will try to acquire the virDomainObjPtr mutex next */
> -    if (eof) {
> -        void (*eofNotify)(qemuAgentPtr, virDomainObjPtr)
> -            = mon->cb->eofNotify;
> -        virDomainObjPtr vm = mon->vm;
> -
> -        /* Make sure anyone waiting wakes up now */
> -        virCondSignal(&mon->notify);
> -        virObjectUnlock(mon);
> -        virObjectUnref(mon);
> -        VIR_DEBUG("Triggering EOF callback");
> -        (eofNotify)(mon, vm);
> -    } else if (error) {
> -        void (*errorNotify)(qemuAgentPtr, virDomainObjPtr)
> -            = mon->cb->errorNotify;
> -        virDomainObjPtr vm = mon->vm;
> -
> -        /* Make sure anyone waiting wakes up now */
> -        virCondSignal(&mon->notify);
> -        virObjectUnlock(mon);
> -        virObjectUnref(mon);
> -        VIR_DEBUG("Triggering error callback");
> -        (errorNotify)(mon, vm);
> + error:
> +    if (mon->lastError.code != VIR_ERR_OK) {
> +        /* Already have an error, so clear any new error */
> +        virResetLastError();
>      } else {
> -        virObjectUnlock(mon);
> -        virObjectUnref(mon);
> +        virErrorPtr err = virGetLastError();
> +        if (!err)
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("Error while processing monitor IO"));
> +        virCopyLastError(&mon->lastError);
> +        virResetLastError();
>      }
> +
> +    VIR_DEBUG("Error on monitor %s", NULLSTR(mon->lastError.message));
> +    /* If IO process resulted in an error & we have a message,
> +     * then wakeup that waiter */
> +    if (mon->msg && !mon->msg->finished) {
> +        mon->msg->finished = 1;
> +        virCondSignal(&mon->notify);
> +    }
> +
> +    qemuAgentUpdateWatch(mon);
> +
> +    errorNotify = mon->cb->errorNotify;
> +    vm = mon->vm;
> +
> +    /* Make sure anyone waiting wakes up now */
> +    virCondSignal(&mon->notify);
> +    virObjectUnlock(mon);
> +    virObjectUnref(mon);
> +    (errorNotify)(mon, vm);
>  }
>  
>  
> @@ -732,9 +705,9 @@ qemuAgentOpen(virDomainObjPtr vm,
>  {
>      qemuAgentPtr mon;
>  
> -    if (!cb || !cb->eofNotify) {
> +    if (!cb || !cb->errorNotify) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("EOF notify callback must be supplied"));
> +                       _("error notify callback must be supplied"));
>          return NULL;
>      }
>  
> diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
> index 6dd9c70..76e8772 100644
> --- a/src/qemu/qemu_agent.h
> +++ b/src/qemu/qemu_agent.h
> @@ -36,8 +36,6 @@ typedef qemuAgentCallbacks *qemuAgentCallbacksPtr;
>  struct _qemuAgentCallbacks {
>      void (*destroy)(qemuAgentPtr mon,
>                      virDomainObjPtr vm);
> -    void (*eofNotify)(qemuAgentPtr mon,
> -                      virDomainObjPtr vm);
>      void (*errorNotify)(qemuAgentPtr mon,
>                          virDomainObjPtr vm);
>  };
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 3da1bd5..fe7682e 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -128,12 +128,12 @@ extern virQEMUDriverPtr qemu_driver;
>   * performed
>   */
>  static void
> -qemuProcessHandleAgentEOF(qemuAgentPtr agent,
> -                          virDomainObjPtr vm)
> +qemuProcessHandleAgentError(qemuAgentPtr agent,
> +                            virDomainObjPtr vm)
>  {
>      qemuDomainObjPrivatePtr priv;
>  
> -    VIR_DEBUG("Received EOF from agent on %p '%s'", vm, vm->def->name);
> +    VIR_DEBUG("Received error from agent on %p '%s'", vm, vm->def->name);
>  
>      virObjectLock(vm);
>  
> @@ -145,7 +145,7 @@ qemuProcessHandleAgentEOF(qemuAgentPtr agent,
>      }
>  
>      if (priv->beingDestroyed) {
> -        VIR_DEBUG("Domain is being destroyed, agent EOF is expected");
> +        VIR_DEBUG("Domain is being destroyed, agent error is expected");
>          goto unlock;
>      }
>  
> @@ -161,19 +161,6 @@ qemuProcessHandleAgentEOF(qemuAgentPtr agent,
>  }
>  
>  
> -/*
> - * This is invoked when there is some kind of error
> - * parsing data to/from the agent. The VM can continue
> - * to run, but no further agent commands will be
> - * allowed
> - */
> -static void
> -qemuProcessHandleAgentError(qemuAgentPtr agent,
> -                            virDomainObjPtr vm)
> -{
> -    qemuProcessHandleAgentEOF(agent, vm);
> -}
> -
>  static void qemuProcessHandleAgentDestroy(qemuAgentPtr agent,
>                                            virDomainObjPtr vm)
>  {
> @@ -185,7 +172,6 @@ static void qemuProcessHandleAgentDestroy(qemuAgentPtr agent,
>  
>  static qemuAgentCallbacks agentCallbacks = {
>      .destroy = qemuProcessHandleAgentDestroy,
> -    .eofNotify = qemuProcessHandleAgentEOF,
>      .errorNotify = qemuProcessHandleAgentError,
>  };
>  
> diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c
> index c86a27a..d9b2726 100644
> --- a/tests/qemumonitortestutils.c
> +++ b/tests/qemumonitortestutils.c
> @@ -752,7 +752,6 @@ qemuMonitorTestAgentNotify(qemuAgentPtr agent ATTRIBUTE_UNUSED,
>  
>  
>  static qemuAgentCallbacks qemuMonitorTestAgentCallbacks = {
> -    .eofNotify = qemuMonitorTestAgentNotify,
>      .errorNotify = qemuMonitorTestAgentNotify,
>  };
>  
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]