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

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(-)

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