[libvirt] [PATCH REBASE 3/5] utils: export virCopyError

[John Ferlan]

On 05/07/2018 04:33 AM, Nikolay Shirokovskiy wrote:
> 
> 
> On 04.05.2018 16:58, John Ferlan wrote:
>>
>>
>> On 05/03/2018 03:39 AM, Nikolay Shirokovskiy wrote:
>>>
>>>
>>> On 01.05.2018 01:03, John Ferlan wrote:
>>>>
>>>>
>>>> On 04/18/2018 10:44 AM, Nikolay Shirokovskiy wrote:
>>>>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
>>>>> ---
>>>>>  src/libvirt_private.syms |  1 +
>>>>>  src/util/virerror.c      | 12 +++++++++---
>>>>>  src/util/virerror.h      |  1 +
>>>>>  3 files changed, 11 insertions(+), 3 deletions(-)
>>>>>
>>>>
>>>> NACK, you should be using virErrorCopyNew
>>>>
>>>> John
>>>
>>> I introduced monError in qemuDomainObjPrivatePtr in next patch and it is not a pointer so
>>> I need this function. I did not make monError pointer for the same reason it is not pointer
>>> in monitor object - I use monError both as flag and as error message.
>>>
>>
>> I saw what you did - the fact is virCopyError is coded as private to the
>> module. Just making it global because you have a use for it is I believe
>> incorrect.
> 
> But why?
> 

Because virErrorCopyNew is designated to "externally" use virCopyError.

>>
>> Ironically in the next patch you have:
>>
>> +    virErrorPtr err = qemuMonitorLastError(mon);
>> +
>> +    virCopyError(err, &priv->monError);
>>
>>
>> The first call isn't guaranteed to set @err and all you're essentially
>> doing is wanting to copy from mon->lastError to priv->monError or
>> perhaps similar to virCopyLastError.
> 
> It is ok that error can turn from non-NULL to NULL on copy due to OOM
> conditions or whaever. It is just as in previous patch. We use priv->monError
> for 2 purpuses. And thus qemuProcessNotifyMonitorError sets priv->monError.code
> explicitly if it still set to VIR_ERR_OK after copy.
> 

It's "possible" from the above code that @priv->monError would have
nothing filled in based on virCopyError logic, so how is that better
than what's being done now?

That's why I figured that changing the innards of virCopyLastError would
be more beneficial in the long run.

>>
>> Maybe some new API in virerror.c should handle that.
>>
> 
> Not sure we need it at this point. But may be I miss something, please
> share your vision in more detail then.
> 

It's not my patch - I'll review whatever comes next. I've provided
suggestions and comments.

John