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

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