[RFC] qemu_migration: Fix virConnectOpenAuth error code
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
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
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
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.
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
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.
participants (3)
-
Ani Sinha -
Michal Prívozník -
Raphael Norwitz