On 07/02/2018 07:33 AM, Nikolay Shirokovskiy wrote: > virCopyLastError is intended to be used after last error is set. > However due to virLastErrorObject failures (very unlikely through > as thread local error is allocated on first use) we can have zero > fields in a copy as a result. In particular code field can be set > to VIR_ERR_OK. > > In some places (qemu monitor, qemu agent and qemu migaration code > for example) we use copy result as a flag and this leads to bugs. > > Let's set OOM-like error in copy in case of virLastErrorObject failures. > > Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; > --- > > Changes from v1: > - check @to > - set OMM error instead of using virErrorGenericFailure > > src/util/virerror.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/src/util/virerror.c b/src/util/virerror.c > index f198f27..737ea92 100644 > --- a/src/util/virerror.c > +++ b/src/util/virerror.c > @@ -366,19 +366,25 @@ virSetError(virErrorPtr newerr) > * > * One will need to free the result with virResetError() > * > - * Returns 0 if no error was found and the error code otherwise and -1 in case > - * of parameter error. > + * Returns error code or -1 in case of parameter error. > */ > int > virCopyLastError(virErrorPtr to) > { > virErrorPtr err = virLastErrorObject(); > + > + if (!to) > + return -1; > + > /* We can't guarantee caller has initialized it to zero */ > memset(to, 0, sizeof(*to)); > - if (err) > + if (err) { > virCopyError(err, to); > - else > - virResetError(to); > + } else { > + to->code = VIR_ERR_NO_MEMORY; > + to->domain = VIR_FROM_NONE; > + to->level = VIR_ERR_ERROR; Should we do a VIR_FREE(to->message); so that nothing that was there before somehow remains... I don't think either agent or monitor> "lastError" is reset until Dispose time.

Beyond that, I think this works for this edge/odd/bad case... Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John > + } > return to->code; > } > >

On 07/19/2018 04:15 AM, Nikolay Shirokovskiy wrote: > > > On 17.07.2018 22:28, John Ferlan wrote: >> >> >>>>> + } else { >>>>> + to->code = VIR_ERR_NO_MEMORY; >>>>> + to->domain = VIR_FROM_NONE; >>>>> + to->level = VIR_ERR_ERROR; >>>> >>>> Should we do a VIR_FREE(to->message); so that nothing that was there >>>> before somehow remains... I don't think either agent or monitor> "lastError" is reset until Dispose time. >>> >>> Won't hurt but probably will not be used by monitor or agent. If thread >>> error is not allocated message is NULL upon return, after error is allocated we never >>> hit this OOM branch anymore. Of course hypotetical client can bring @to >>> with message already set so this a bit future proof. >>> >>> I think then we can leave reset and then set these 3 fields. >>> >>> Nikolay >>> >> >> You have commit access and my R-by regardless of whether you add the >> VIR_FREE or not. I leave the rest to you ;-) >> > > Thanx! Pushed with reset added. > > Nikolay > Oh no, you pushed a local .gnulib too! git log -p ... commit 1bff5bbe25eb7a7e7a4e0067c4ca7cbc1cb34999 Author: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; Date: Mon Jul 2 14:16:52 2018 +0300 ... diff --git a/.gnulib b/.gnulib index cdbf3d385a..d6397dde2e 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit cdbf3d385a32ff904c96f20c26f3470bd8345248 +Subproject commit d6397dde2e127e246e3eeb5254a21f42cac783c8 diff --git a/src/util/virerror.c b/src/util/virerror.c index f198f27957..5f26d59777 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c ...