1. Patchew
  2. Libvirt
  3. View series

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

Nikolay Shirokovskiy posted 1 patch 5 years, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1526551903-267397-1-git-send-email-nshirokovskiy@virtuozzo.com
Test syntax-check passed
src/util/virerror.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[libvirt] [PATCH] util: return generic error in virCopyLastError if error is not set
Posted by Nikolay Shirokovskiy 5 years, 11 months ago 
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);
     return to->code;
 }
 
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: return generic error in virCopyLastError if error is not set
Posted by Nikolay Shirokovskiy 5 years, 11 months ago 

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

>      return to->code;
>  }
>  
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: return generic error in virCopyLastError if error is not set
Posted by Erik Skultety 5 years, 11 months ago 
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
caller-passed object, since we just blindly trust it's been allocated properly
and access it right away.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: return generic error in virCopyLastError if error is not set
Posted by Nikolay Shirokovskiy 5 years, 11 months ago 

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: return generic error in virCopyLastError if error is not set
Posted by Nikolay Shirokovskiy 5 years, 11 months ago 

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.

Nikolay


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: return generic error in virCopyLastError if error is not set
Posted by Erik Skultety 5 years, 11 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: return generic error in virCopyLastError if error is not set
Posted by Nikolay Shirokovskiy 5 years, 9 months ago 

On 18.05.2018 13:14, Erik Skultety wrote:
> 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
> 

Hi, Erik. While this approach looks interesting it takes too much effort
to use with thread pools. We need to teach thread pool to deal with threads
than fail to initialize. It is much simplier to adopt virCopyLastError for
such a corner case. Of course we still can have issues with virErrorPreserveLast/virErrorRestore
but they are not critical - client just get some unrelated error but 
daemon will continue to work properly. So I sent v2 version of this patch with
changes as you suggested.

Nikolay

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: return generic error in virCopyLastError if error is not set
Posted by Erik Skultety 5 years, 11 months ago 
On Thu, May 17, 2018 at 02:49:12PM +0300, 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

Although true, how does VIR_ERR_UNKNOWN help to tell you anything about the
original root cause, if you at least report OOM error, of course the root cause
might have been different, but at least it conveys some information that is
true, especially with OOM which tells you to "buckle up" because things are
going haywire - on the contrary, if we report UNKNOWN, you convey no
information plus people will treat is as an unhandled exception, shrug their
shoulders and file a bugzilla and paste the error there, which without any
debug logs is good as nothing AND, since we're talking OOM here, things still
will go haywire. I don't think this patch offers any improvement over the
existing problem.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: return generic error in virCopyLastError if error is not set
Posted by Nikolay Shirokovskiy 5 years, 11 months ago 

On 17.05.2018 15:25, Erik Skultety wrote:
> On Thu, May 17, 2018 at 02:49:12PM +0300, 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
> 
> Although true, how does VIR_ERR_UNKNOWN help to tell you anything about the
> original root cause, if you at least report OOM error, of course the root cause
> might have been different, but at least it conveys some information that is
> true, especially with OOM which tells you to "buckle up" because things are
> going haywire - on the contrary, if we report UNKNOWN, you convey no
> information plus people will treat is as an unhandled exception, shrug their
> shoulders and file a bugzilla and paste the error there, which without any
> debug logs is good as nothing AND, since we're talking OOM here, things still

Well, makes sense.

> will go haywire. I don't think this patch offers any improvement over the
> existing problem.

Wait, but we need to set .code to some error otherwise the logic of callers
will be broken (see qemu monitor for example).

Please check another approach suggested in my another reply.

Nikolay

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: return generic error in virCopyLastError if error is not set
Posted by Erik Skultety 5 years, 11 months ago 
On Thu, May 17, 2018 at 03:35:59PM +0300, Nikolay Shirokovskiy wrote:
>
>
> On 17.05.2018 15:25, Erik Skultety wrote:
> > On Thu, May 17, 2018 at 02:49:12PM +0300, 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
> >
> > Although true, how does VIR_ERR_UNKNOWN help to tell you anything about the
> > original root cause, if you at least report OOM error, of course the root cause
> > might have been different, but at least it conveys some information that is
> > true, especially with OOM which tells you to "buckle up" because things are
> > going haywire - on the contrary, if we report UNKNOWN, you convey no
> > information plus people will treat is as an unhandled exception, shrug their
> > shoulders and file a bugzilla and paste the error there, which without any
> > debug logs is good as nothing AND, since we're talking OOM here, things still
>
> Well, makes sense.
>
> > will go haywire. I don't think this patch offers any improvement over the
> > existing problem.
>
> Wait, but we need to set .code to some error otherwise the logic of callers
> will be broken (see qemu monitor for example).

Yeah, that's why I suggested setting .code to VIR_ERR_NO_MEMORY..

>
> Please check another approach suggested in my another reply.

Sure, I'll have a look at it tomorrow and respond in order to move this forward.

Thanks,
Erik

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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.