[libvirt] [PATCH v2] util: set OOM in virCopyLastError if error is not set

[Nikolay Shirokovskiy]

On 17.07.2018 01:38, John Ferlan wrote:
> 
> 
> 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 at virtuozzo.com>
>> ---
>>
>> 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.

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

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