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

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

 




On 26.10.2016 23:48, John Ferlan wrote:
> 
> 
> 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.

I would not put the change this way. It is just doesn't matter for
us - EOF or error. The cause is different, the result is same. 
Then why keep this overwhelming logic in event loop? Just compare 
qemuAgentIO before and after the patch...


Nikolay

> 
>> 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]