[libvirt] [PATCH] util: return generic error in virCopyLastError if error is not set

[Erik Skultety]

On Thu, May 17, 2018 at 03:24:49PM +0300, Nikolay Shirokovskiy wrote:
>
>
> On 17.05.2018 14:49, Nikolay Shirokovskiy wrote:
> >
> >
> > On 17.05.2018 14:01, Erik Skultety wrote:
> >> On Thu, May 17, 2018 at 01:42:36PM +0300, Nikolay Shirokovskiy wrote:
> >>>
> >>>
> >>> On 17.05.2018 13:11, 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 copy result to generic error ("cause unknown") in this case.
> >>>> Approach is based on John Ferlan comments in [1] and [2] to initial
> >>>> attempt which fixes issue in much less generic way.
> >>>>
> >>>> [1] https://www.redhat.com/archives/libvir-list/2018-April/msg02700.html
> >>>> [2] https://www.redhat.com/archives/libvir-list/2018-May/msg00124.html
> >>>> ---
> >>>>  src/util/virerror.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/src/util/virerror.c b/src/util/virerror.c
> >>>> index c000b00..9f158af 100644
> >>>> --- a/src/util/virerror.c
> >>>> +++ b/src/util/virerror.c
> >>>> @@ -343,7 +343,7 @@ virCopyLastError(virErrorPtr to)
> >>>>      if (err)
> >>>>          virCopyError(err, to);
> >>>>      else
> >>>> -        virResetError(to);
> >>>> +        virErrorGenericFailure(err);
> >>>
> >>> virErrorGenericFailure(to) of course
> >>
> >> Yeah, I just prepared a mail to respond to the original patch, nevermind :)...
> >> Although I still don't see how this solves anything. First of all, the whole
> >> else branch is useless, since we "memset'd" the caller-passed object right
> >> before we could potentially hit the 'else' branch. As you've noted in the
> >> commit message, virLastErrorObject() can only fail with NULL on OOM, yet one of
> >> the things virErrorGenericFailure will try to do is strdup(), which I highly
> >> doubt would succeed after a previous OOM, IOW if libvirt encounters OOM it's
> >> pretty much stuck in a loop of failed attempts to report an OOM error
> >> encountering some more. If you really want to return something, I'd say we
> >> should return VIR_ERR_NO_MEMORY instead, probably checking the validity of the
> >
> > This can be misleading. The original error is lost due OOM but it can be
> > not OOM itself and we tend to report root cause. I'd prefer new VIR_ERR_UNKNOWN code
> > if sematics is important. Then we need to fix virErrorGenericFailure too
> > as VIR_ERR_INTERNAL_ERROR code is valid only if we forget to set error but
> > we can also have OOM issues too in this case, so VIR_ERR_UNKNOWN is better suited
> > for virErrorGenericFailure. Or we can live with just VIR_ERR_INTERNAL_ERROR.
> >
> > As to duping message - it won't hurt anyway. So I would keep virErrorGenericFailure
> > usage.
> >
> >> caller-passed object, since we just blindly trust it's been allocated properly
> >> and access it right away.
> >>
> >
> > As to checking @to validity, I'd prefer a distinct patch.
> >
> > Nikolay
> >
> >
>
> Eventually I think we just need to initialize thread local error storage
> in the very beginning of thread life. Then we don't have this corner
> case. And similar others too. Check for example virErrorPreserveLast/virErrorRestore.
> They don't account that last error can be NULL because of OOM so last
> error can be overwritten.

I think it would make sense for us to create the error object right at the
thread creation time rather than ad-hoc when we actually need it, although, how
likely is it that we're going to hit such a corner case, but sure, feel free to
post patches for that.

Erik