1. Patchew
  2. Libvirt
  3. View series

[RFC] qemu_migration: Fix virConnectOpenAuth error code

Raphael Norwitz posted 1 patch 2 years, 3 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20220106013316.18264-1-raphael.norwitz@nutanix.com
src/qemu/qemu_migration.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[RFC] qemu_migration: Fix virConnectOpenAuth error code
Posted by Raphael Norwitz 2 years, 3 months ago 
Today if virConnectOpenAuth fails, qemuMigrationSrcPerformPeer2Peer()
returns VIR_ERR_OPERATION_FAILED. This change switches that error code
to VIR_ERR_NO_CONNECT, which is more accurate.

This should help libvirt consumers more intellegently retry migrations
on intermittent connection failures.

CC: Bhuvnesh Jain <bhuvnesh.jain@nutanix.com>
Suggested-by: John Levon <john.levon@nutanix.com>
Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
---
 src/qemu/qemu_migration.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index b9d7d582f5..f7ced209a4 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -5145,7 +5145,7 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriver *driver,
         goto cleanup;
 
     if (dconn == NULL) {
-        virReportError(VIR_ERR_OPERATION_FAILED,
+        virReportError(VIR_ERR_NO_CONNECT,
                        _("Failed to connect to remote libvirt URI %s: %s"),
                        dconnuri, virGetLastErrorMessage());
         return -1;
-- 
2.20.1
Re: [RFC] qemu_migration: Fix virConnectOpenAuth error code
Posted by Ani Sinha 2 years, 3 months ago 

On Thu, 6 Jan 2022, Raphael Norwitz wrote:

> Today if virConnectOpenAuth fails, qemuMigrationSrcPerformPeer2Peer()
> returns VIR_ERR_OPERATION_FAILED. This change switches that error code
> to VIR_ERR_NO_CONNECT, which is more accurate.

Hmm, I am not sure if this would be the right thing.
virConnectOpenInternal() fails under many conditions, two of which
actiually qualifies for VIR_ERR_NO_CONNECT. This error symbolizes "no
connection driver available" which to me has a much narrower scope.
virConnectOpenInternal() has other failure codepaths, for example
VIR_ERR_CONFIG_UNSUPPORTED. Catching all those failures as
VIR_ERR_OPERATION_FAILED that has a wider scope seems to be correct thing
to do in the existing code.

YMMV.

>
> This should help libvirt consumers more intellegently retry migrations
> on intermittent connection failures.
>
> CC: Bhuvnesh Jain <bhuvnesh.jain@nutanix.com>
> Suggested-by: John Levon <john.levon@nutanix.com>
> Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> ---
>  src/qemu/qemu_migration.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index b9d7d582f5..f7ced209a4 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -5145,7 +5145,7 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriver *driver,
>          goto cleanup;
>
>      if (dconn == NULL) {
> -        virReportError(VIR_ERR_OPERATION_FAILED,
> +        virReportError(VIR_ERR_NO_CONNECT,
>                         _("Failed to connect to remote libvirt URI %s: %s"),
>                         dconnuri, virGetLastErrorMessage());
>          return -1;
> --
> 2.20.1
>
>
>
Re: [RFC] qemu_migration: Fix virConnectOpenAuth error code
Posted by Michal Prívozník 2 years, 3 months ago 
On 1/6/22 02:33, Raphael Norwitz wrote:
> Today if virConnectOpenAuth fails, qemuMigrationSrcPerformPeer2Peer()
> returns VIR_ERR_OPERATION_FAILED. This change switches that error code
> to VIR_ERR_NO_CONNECT, which is more accurate.
> 
> This should help libvirt consumers more intellegently retry migrations
> on intermittent connection failures.
> 
> CC: Bhuvnesh Jain <bhuvnesh.jain@nutanix.com>
> Suggested-by: John Levon <john.levon@nutanix.com>
> Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> ---
>  src/qemu/qemu_migration.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index b9d7d582f5..f7ced209a4 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -5145,7 +5145,7 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriver *driver,
>          goto cleanup;
>  
>      if (dconn == NULL) {
> -        virReportError(VIR_ERR_OPERATION_FAILED,
> +        virReportError(VIR_ERR_NO_CONNECT,
>                         _("Failed to connect to remote libvirt URI %s: %s"),
>                         dconnuri, virGetLastErrorMessage());
>          return -1;

I'm not exactly sure why we need this virReportError() in the first
place. Basically, we are just overwriting a more accurate error with
this generic one. Doesn't removing this virReportError() fix the problem?

Michal
Re: [RFC] qemu_migration: Fix virConnectOpenAuth error code
Posted by Ani Sinha 2 years, 3 months ago 

On Thu, 6 Jan 2022, Michal Prívozník wrote:

> On 1/6/22 02:33, Raphael Norwitz wrote:
> > Today if virConnectOpenAuth fails, qemuMigrationSrcPerformPeer2Peer()
> > returns VIR_ERR_OPERATION_FAILED. This change switches that error code
> > to VIR_ERR_NO_CONNECT, which is more accurate.
> >
> > This should help libvirt consumers more intellegently retry migrations
> > on intermittent connection failures.
> >
> > CC: Bhuvnesh Jain <bhuvnesh.jain@nutanix.com>
> > Suggested-by: John Levon <john.levon@nutanix.com>
> > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> > ---
> >  src/qemu/qemu_migration.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index b9d7d582f5..f7ced209a4 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> > @@ -5145,7 +5145,7 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriver *driver,
> >          goto cleanup;
> >
> >      if (dconn == NULL) {
> > -        virReportError(VIR_ERR_OPERATION_FAILED,
> > +        virReportError(VIR_ERR_NO_CONNECT,
> >                         _("Failed to connect to remote libvirt URI %s: %s"),
> >                         dconnuri, virGetLastErrorMessage());
> >          return -1;
>
> I'm not exactly sure why we need this virReportError() in the first
> place. Basically, we are just overwriting a more accurate error with
> this generic one. Doesn't removing this virReportError() fix the problem?

Not all failure scenarios in virConnectOpenInternal() are
caught with a virReportError(). Hence, we do need this.
Re: [RFC] qemu_migration: Fix virConnectOpenAuth error code
Posted by Michal Prívozník 2 years, 3 months ago 
On 1/6/22 13:42, Ani Sinha wrote:
> 
> 
> On Thu, 6 Jan 2022, Michal Prívozník wrote:
> 
>> On 1/6/22 02:33, Raphael Norwitz wrote:
>>> Today if virConnectOpenAuth fails, qemuMigrationSrcPerformPeer2Peer()
>>> returns VIR_ERR_OPERATION_FAILED. This change switches that error code
>>> to VIR_ERR_NO_CONNECT, which is more accurate.
>>>
>>> This should help libvirt consumers more intellegently retry migrations
>>> on intermittent connection failures.
>>>
>>> CC: Bhuvnesh Jain <bhuvnesh.jain@nutanix.com>
>>> Suggested-by: John Levon <john.levon@nutanix.com>
>>> Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
>>> ---
>>>  src/qemu/qemu_migration.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>>> index b9d7d582f5..f7ced209a4 100644
>>> --- a/src/qemu/qemu_migration.c
>>> +++ b/src/qemu/qemu_migration.c
>>> @@ -5145,7 +5145,7 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriver *driver,
>>>          goto cleanup;
>>>
>>>      if (dconn == NULL) {
>>> -        virReportError(VIR_ERR_OPERATION_FAILED,
>>> +        virReportError(VIR_ERR_NO_CONNECT,
>>>                         _("Failed to connect to remote libvirt URI %s: %s"),
>>>                         dconnuri, virGetLastErrorMessage());
>>>          return -1;
>>
>> I'm not exactly sure why we need this virReportError() in the first
>> place. Basically, we are just overwriting a more accurate error with
>> this generic one. Doesn't removing this virReportError() fix the problem?
> 
> Not all failure scenarios in virConnectOpenInternal() are
> caught with a virReportError(). Hence, we do need this.

Well, then the problem is in virConnectOpenInternal(). Either it should
report error in all cases or none, because then the caller would have to
check if error was reported or not.

Michal
Re: [RFC] qemu_migration: Fix virConnectOpenAuth error code
Posted by Ani Sinha 2 years, 3 months ago 

On Thu, 6 Jan 2022, Michal Prívozník wrote:

> On 1/6/22 13:42, Ani Sinha wrote:
> >
> >
> > On Thu, 6 Jan 2022, Michal Prívozník wrote:
> >
> >> On 1/6/22 02:33, Raphael Norwitz wrote:
> >>> Today if virConnectOpenAuth fails, qemuMigrationSrcPerformPeer2Peer()
> >>> returns VIR_ERR_OPERATION_FAILED. This change switches that error code
> >>> to VIR_ERR_NO_CONNECT, which is more accurate.
> >>>
> >>> This should help libvirt consumers more intellegently retry migrations
> >>> on intermittent connection failures.
> >>>
> >>> CC: Bhuvnesh Jain <bhuvnesh.jain@nutanix.com>
> >>> Suggested-by: John Levon <john.levon@nutanix.com>
> >>> Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> >>> ---
> >>>  src/qemu/qemu_migration.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> >>> index b9d7d582f5..f7ced209a4 100644
> >>> --- a/src/qemu/qemu_migration.c
> >>> +++ b/src/qemu/qemu_migration.c
> >>> @@ -5145,7 +5145,7 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriver *driver,
> >>>          goto cleanup;
> >>>
> >>>      if (dconn == NULL) {
> >>> -        virReportError(VIR_ERR_OPERATION_FAILED,
> >>> +        virReportError(VIR_ERR_NO_CONNECT,
> >>>                         _("Failed to connect to remote libvirt URI %s: %s"),
> >>>                         dconnuri, virGetLastErrorMessage());
> >>>          return -1;
> >>
> >> I'm not exactly sure why we need this virReportError() in the first
> >> place. Basically, we are just overwriting a more accurate error with
> >> this generic one. Doesn't removing this virReportError() fix the problem?
> >
> > Not all failure scenarios in virConnectOpenInternal() are
> > caught with a virReportError(). Hence, we do need this.
>
> Well, then the problem is in virConnectOpenInternal(). Either it should
> report error in all cases or none, because then the caller would have to
> check if error was reported or not.
>

OK fine. I will post a patch soon.

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.