[libvirt] [PATCH 07/10] qemu: agent: simplify io loop
John Ferlan
jferlan at redhat.com
Wed Oct 26 20:48:37 UTC 2016
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,
> };
>
>
More information about the libvir-list
mailing list