1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH REBASE 0/5] qemu: fix domain object wait to handle monitor errors
  4. View patch

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

Nikolay Shirokovskiy posted 5 patches 7 years ago
[libvirt] [PATCH REBASE 3/5] utils: export virCopyError
Posted by Nikolay Shirokovskiy 7 years ago 
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
---
 src/libvirt_private.syms |  1 +
 src/util/virerror.c      | 12 +++++++++---
 src/util/virerror.h      |  1 +
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b31f599..6bbbf77 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1702,6 +1702,7 @@ ebtablesRemoveForwardAllowIn;
 
 
 # util/virerror.h
+virCopyError;
 virDispatchError;
 virErrorCopyNew;
 virErrorInitialize;
diff --git a/src/util/virerror.c b/src/util/virerror.c
index c000b00..2ff8a3e 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -188,10 +188,16 @@ virErrorGenericFailure(virErrorPtr err)
 }
 
 
-/*
- * Internal helper to perform a deep copy of an error
+/**
+ * virCopyError:
+ * @from: error to copy from
+ * @to: error to copy to
+ *
+ * Copy error fields from @from to @to.
+ *
+ * Returns 0 on success, -1 on failure.
  */
-static int
+int
 virCopyError(virErrorPtr from,
              virErrorPtr to)
 {
diff --git a/src/util/virerror.h b/src/util/virerror.h
index 760bfa4..90ac619 100644
--- a/src/util/virerror.h
+++ b/src/util/virerror.h
@@ -192,6 +192,7 @@ void virReportOOMErrorFull(int domcode,
 
 int virSetError(virErrorPtr newerr);
 virErrorPtr virErrorCopyNew(virErrorPtr err);
+int virCopyError(virErrorPtr from, virErrorPtr to);
 void virDispatchError(virConnectPtr conn);
 const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen);
 
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH REBASE 3/5] utils: export virCopyError
Posted by John Ferlan 7 years ago 

On 04/18/2018 10:44 AM, Nikolay Shirokovskiy wrote:
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@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

> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index b31f599..6bbbf77 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1702,6 +1702,7 @@ ebtablesRemoveForwardAllowIn;
>  
>  
>  # util/virerror.h
> +virCopyError;
>  virDispatchError;
>  virErrorCopyNew;
>  virErrorInitialize;
> diff --git a/src/util/virerror.c b/src/util/virerror.c
> index c000b00..2ff8a3e 100644
> --- a/src/util/virerror.c
> +++ b/src/util/virerror.c
> @@ -188,10 +188,16 @@ virErrorGenericFailure(virErrorPtr err)
>  }
>  
>  
> -/*
> - * Internal helper to perform a deep copy of an error
> +/**
> + * virCopyError:
> + * @from: error to copy from
> + * @to: error to copy to
> + *
> + * Copy error fields from @from to @to.
> + *
> + * Returns 0 on success, -1 on failure.
>   */
> -static int
> +int
>  virCopyError(virErrorPtr from,
>               virErrorPtr to)
>  {
> diff --git a/src/util/virerror.h b/src/util/virerror.h
> index 760bfa4..90ac619 100644
> --- a/src/util/virerror.h
> +++ b/src/util/virerror.h
> @@ -192,6 +192,7 @@ void virReportOOMErrorFull(int domcode,
>  
>  int virSetError(virErrorPtr newerr);
>  virErrorPtr virErrorCopyNew(virErrorPtr err);
> +int virCopyError(virErrorPtr from, virErrorPtr to);
>  void virDispatchError(virConnectPtr conn);
>  const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen);
>  
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH REBASE 3/5] utils: export virCopyError
Posted by Nikolay Shirokovskiy 7 years ago 

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@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.

> 
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index b31f599..6bbbf77 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -1702,6 +1702,7 @@ ebtablesRemoveForwardAllowIn;
>>  
>>  
>>  # util/virerror.h
>> +virCopyError;
>>  virDispatchError;
>>  virErrorCopyNew;
>>  virErrorInitialize;
>> diff --git a/src/util/virerror.c b/src/util/virerror.c
>> index c000b00..2ff8a3e 100644
>> --- a/src/util/virerror.c
>> +++ b/src/util/virerror.c
>> @@ -188,10 +188,16 @@ virErrorGenericFailure(virErrorPtr err)
>>  }
>>  
>>  
>> -/*
>> - * Internal helper to perform a deep copy of an error
>> +/**
>> + * virCopyError:
>> + * @from: error to copy from
>> + * @to: error to copy to
>> + *
>> + * Copy error fields from @from to @to.
>> + *
>> + * Returns 0 on success, -1 on failure.
>>   */
>> -static int
>> +int
>>  virCopyError(virErrorPtr from,
>>               virErrorPtr to)
>>  {
>> diff --git a/src/util/virerror.h b/src/util/virerror.h
>> index 760bfa4..90ac619 100644
>> --- a/src/util/virerror.h
>> +++ b/src/util/virerror.h
>> @@ -192,6 +192,7 @@ void virReportOOMErrorFull(int domcode,
>>  
>>  int virSetError(virErrorPtr newerr);
>>  virErrorPtr virErrorCopyNew(virErrorPtr err);
>> +int virCopyError(virErrorPtr from, virErrorPtr to);
>>  void virDispatchError(virConnectPtr conn);
>>  const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen);
>>  
>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH REBASE 3/5] utils: export virCopyError
Posted by John Ferlan 7 years ago 

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@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.

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.

Maybe some new API in virerror.c should handle that.

John

>>
>>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>>> index b31f599..6bbbf77 100644
>>> --- a/src/libvirt_private.syms
>>> +++ b/src/libvirt_private.syms
>>> @@ -1702,6 +1702,7 @@ ebtablesRemoveForwardAllowIn;
>>>  
>>>  
>>>  # util/virerror.h
>>> +virCopyError;
>>>  virDispatchError;
>>>  virErrorCopyNew;
>>>  virErrorInitialize;
>>> diff --git a/src/util/virerror.c b/src/util/virerror.c
>>> index c000b00..2ff8a3e 100644
>>> --- a/src/util/virerror.c
>>> +++ b/src/util/virerror.c
>>> @@ -188,10 +188,16 @@ virErrorGenericFailure(virErrorPtr err)
>>>  }
>>>  
>>>  
>>> -/*
>>> - * Internal helper to perform a deep copy of an error
>>> +/**
>>> + * virCopyError:
>>> + * @from: error to copy from
>>> + * @to: error to copy to
>>> + *
>>> + * Copy error fields from @from to @to.
>>> + *
>>> + * Returns 0 on success, -1 on failure.
>>>   */
>>> -static int
>>> +int
>>>  virCopyError(virErrorPtr from,
>>>               virErrorPtr to)
>>>  {
>>> diff --git a/src/util/virerror.h b/src/util/virerror.h
>>> index 760bfa4..90ac619 100644
>>> --- a/src/util/virerror.h
>>> +++ b/src/util/virerror.h
>>> @@ -192,6 +192,7 @@ void virReportOOMErrorFull(int domcode,
>>>  
>>>  int virSetError(virErrorPtr newerr);
>>>  virErrorPtr virErrorCopyNew(virErrorPtr err);
>>> +int virCopyError(virErrorPtr from, virErrorPtr to);
>>>  void virDispatchError(virConnectPtr conn);
>>>  const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen);
>>>  
>>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH REBASE 3/5] utils: export virCopyError
Posted by Nikolay Shirokovskiy 7 years ago 

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@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?

> 
> 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.

> 
> 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.

Nikolay

> 
>>>
>>>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>>>> index b31f599..6bbbf77 100644
>>>> --- a/src/libvirt_private.syms
>>>> +++ b/src/libvirt_private.syms
>>>> @@ -1702,6 +1702,7 @@ ebtablesRemoveForwardAllowIn;
>>>>  
>>>>  
>>>>  # util/virerror.h
>>>> +virCopyError;
>>>>  virDispatchError;
>>>>  virErrorCopyNew;
>>>>  virErrorInitialize;
>>>> diff --git a/src/util/virerror.c b/src/util/virerror.c
>>>> index c000b00..2ff8a3e 100644
>>>> --- a/src/util/virerror.c
>>>> +++ b/src/util/virerror.c
>>>> @@ -188,10 +188,16 @@ virErrorGenericFailure(virErrorPtr err)
>>>>  }
>>>>  
>>>>  
>>>> -/*
>>>> - * Internal helper to perform a deep copy of an error
>>>> +/**
>>>> + * virCopyError:
>>>> + * @from: error to copy from
>>>> + * @to: error to copy to
>>>> + *
>>>> + * Copy error fields from @from to @to.
>>>> + *
>>>> + * Returns 0 on success, -1 on failure.
>>>>   */
>>>> -static int
>>>> +int
>>>>  virCopyError(virErrorPtr from,
>>>>               virErrorPtr to)
>>>>  {
>>>> diff --git a/src/util/virerror.h b/src/util/virerror.h
>>>> index 760bfa4..90ac619 100644
>>>> --- a/src/util/virerror.h
>>>> +++ b/src/util/virerror.h
>>>> @@ -192,6 +192,7 @@ void virReportOOMErrorFull(int domcode,
>>>>  
>>>>  int virSetError(virErrorPtr newerr);
>>>>  virErrorPtr virErrorCopyNew(virErrorPtr err);
>>>> +int virCopyError(virErrorPtr from, virErrorPtr to);
>>>>  void virDispatchError(virConnectPtr conn);
>>>>  const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen);
>>>>  
>>>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH REBASE 3/5] utils: export virCopyError
Posted by John Ferlan 7 years ago 

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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.