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