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

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.

> 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, > }; > >