Re: [libvirt] [PATCH REBASE 2/5] qemu: monitor: set error flag even in OOM conditions

On 01.05.2018 01:03, John Ferlan wrote: > > > On 04/18/2018 10:44 AM, Nikolay Shirokovskiy wrote: >> lastError.code is used as flag in qemu monitor. Unfortunately >> due to temporary OOM conditions (very unlikely through as thread local >> error is allocated on first use) we can fail to set this flag >> in case of monitor eofs/errors. This can cause hangs. >> >> Let's make sure flag is always set in case eofs/errors. >> >> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; >> --- >> src/qemu/qemu_monitor.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c >> index f642d9a..906a320 100644 >> --- a/src/qemu/qemu_monitor.c >> +++ b/src/qemu/qemu_monitor.c >> @@ -757,6 +757,11 @@ qemuMonitorIO(int watch, int fd, int events, void *opaque) >> virReportError(VIR_ERR_INTERNAL_ERROR, "%s", >> _("Error while processing monitor IO")); >> virCopyLastError(&mon->lastError); >> + >> + /* set error code if due to OOM conditions we fail to set it before */ >> + if (mon->lastError.code == VIR_ERR_OK) > > So this can only happen if virCopyLastError fails, right? And code == 0 Nope, virCopyLastError fail to set code field only in one case - if last error is NULL which can be only if in next lines even reporting fails: virErrorPtr err = virGetLastError(); if (!err) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Error while processing monitor IO"));

> because of memset(..., 0, ...). Wouldn't you only be working around the > issue for one of the callers? and only setting *.code and not *.domain? We use mon->lastError for 2 purposes: - as flag, and it is critical that we set this flag appropriately. We use only code field for this purpuse

- as error message that client can retrieve in case if API call returns error, it is no critical if we can not provide error message, moreover we can fail to pass error message in RPC for example due to temporary OOM conditions I even think to remove flag usage from mon->lastError and introduce bool flag but this looks ugly like 2 fields to keep error.

> > BTW: I feel we've been this way in rather recent history... See commit > 'e711a391' - I recall we had varying opinions on that one. Yes, but this fix does not relate to usage mon->lastError as flag, rather then to second usage in the above distinction. > > Anyway, assuming CopyLastError is the issue, then perhaps the right > place to fix this is there and I would think at least the following > would need to be set in the error path: > > to->code = VIR_ERR_NO_MEMORY; > to->domain = VIR_FROM_THREAD; It can be misleading. For example if we forget to set last error and then try to copy it. > > We cannot create a to->message... > > Of course, how can we be "sure" that the failure is because the > VIR_ALLOC_QUIET in virLastErrorObject caused the issue other than > changing it's return values or passing a "bool &memfail" that's only set > when the VIR_ALLOC_QUIET fails allowing the caller to decide what to do. > It's local, so change is limited. Thus virGetLastErrorMessage and > virCopyLastError could make use of the returned, while other callers > could just pass "&dummy". > > In the long run, it's a somewhat interesting corner case problem and we > may have some varying opinions over the "best way" to resolve. Suffice > to say, libvirt probably isn't going to "survive" much longer, but > unless others read this response and have varying opinions and/or we > come to a resolution - can this be posted/consider on it's own merits?> > Tks - > > John May be it would be just more simple to not to use mon->lastError as flag in monitor.

> >> + mon->lastError.code = VIR_ERR_INTERNAL_ERROR; >> + >> virResetLastError(); >> } >> >>