[PATCH] esxConnectListAllDomains: Don't propagate failure to lookup a single domain
From: Peter Krempa <pkrempa@redhat.com> In esxConnectListAllDomains if the lookup of the VM name and UUID fails for a single VM (possible e.g. with broken storage) the whole API would return failure even when there are working VMs. Rework the lookup so that if a subset fails we ignore the failure on those. We report an error only if lookup of all of the objects failed. Failure is reported from the last one. Resolves: https://issues.redhat.com/browse/RHEL-80606 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/esx/esx_driver.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 554fb3e18f..d869481698 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4792,18 +4792,21 @@ esxConnectListAllDomains(virConnectPtr conn, virtualMachine = virtualMachine->_next) { g_autofree char *name = NULL; - if (needIdentity) { - if (esxVI_GetVirtualMachineIdentity(virtualMachine, &id, - &name, uuid) < 0) { + /* If the lookup of the required properties fails for some of the machines + * in the list it's preferrable to return the valid objects instead of + * failing outright */ + if ((needIdentity && esxVI_GetVirtualMachineIdentity(virtualMachine, &id, &name, uuid) < 0) || + (needPowerState && esxVI_GetVirtualMachinePowerState(virtualMachine, &powerState) < 0)) { + + /* Raise error from last lookup if we didn't successfuly fetch any + * domain objecst yet */ + if (count == 0 && !virtualMachine->_next) goto cleanup; - } - } - if (needPowerState) { - if (esxVI_GetVirtualMachinePowerState(virtualMachine, - &powerState) < 0) { - goto cleanup; - } + /* failure to fetch information of a single VM must not interrupt + * the lookup of the rest */ + virResetLastError(); + continue; } /* filter by active state */ -- 2.49.0
On Wed, Mar 26, 2025 at 13:39:55 +0100, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
In esxConnectListAllDomains if the lookup of the VM name and UUID fails for a single VM (possible e.g. with broken storage) the whole API would return failure even when there are working VMs.
Rework the lookup so that if a subset fails we ignore the failure on those. We report an error only if lookup of all of the objects failed. Failure is reported from the last one.
Resolves: https://issues.redhat.com/browse/RHEL-80606 Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
Forgot to add disclaimer. I didn't test this because I don't have a (broken) ESX setup.
On a Wednesday in 2025, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
In esxConnectListAllDomains if the lookup of the VM name and UUID fails for a single VM (possible e.g. with broken storage) the whole API would return failure even when there are working VMs.
Rework the lookup so that if a subset fails we ignore the failure on those. We report an error only if lookup of all of the objects failed. Failure is reported from the last one.
Resolves: https://issues.redhat.com/browse/RHEL-80606 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/esx/esx_driver.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 554fb3e18f..d869481698 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4792,18 +4792,21 @@ esxConnectListAllDomains(virConnectPtr conn, virtualMachine = virtualMachine->_next) { g_autofree char *name = NULL;
- if (needIdentity) { - if (esxVI_GetVirtualMachineIdentity(virtualMachine, &id, - &name, uuid) < 0) { + /* If the lookup of the required properties fails for some of the machines + * in the list it's preferrable to return the valid objects instead of + * failing outright */ + if ((needIdentity && esxVI_GetVirtualMachineIdentity(virtualMachine, &id, &name, uuid) < 0) || + (needPowerState && esxVI_GetVirtualMachinePowerState(virtualMachine, &powerState) < 0)) { + + /* Raise error from last lookup if we didn't successfuly fetch any + * domain objecst yet */
typo. Also, the comment does not address the fact that we only raise error if there are no more domains to fetch.
+ if (count == 0 && !virtualMachine->_next) goto cleanup; - } - }
- if (needPowerState) { - if (esxVI_GetVirtualMachinePowerState(virtualMachine, - &powerState) < 0) { - goto cleanup; - } + /* failure to fetch information of a single VM must not interrupt + * the lookup of the rest */ + virResetLastError(); + continue; }
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Wed, Mar 26, 2025 at 15:07:37 +0100, Ján Tomko wrote:
On a Wednesday in 2025, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
In esxConnectListAllDomains if the lookup of the VM name and UUID fails for a single VM (possible e.g. with broken storage) the whole API would return failure even when there are working VMs.
Rework the lookup so that if a subset fails we ignore the failure on those. We report an error only if lookup of all of the objects failed. Failure is reported from the last one.
Resolves: https://issues.redhat.com/browse/RHEL-80606 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/esx/esx_driver.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 554fb3e18f..d869481698 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4792,18 +4792,21 @@ esxConnectListAllDomains(virConnectPtr conn, virtualMachine = virtualMachine->_next) { g_autofree char *name = NULL;
- if (needIdentity) { - if (esxVI_GetVirtualMachineIdentity(virtualMachine, &id, - &name, uuid) < 0) { + /* If the lookup of the required properties fails for some of the machines + * in the list it's preferrable to return the valid objects instead of + * failing outright */ + if ((needIdentity && esxVI_GetVirtualMachineIdentity(virtualMachine, &id, &name, uuid) < 0) || + (needPowerState && esxVI_GetVirtualMachinePowerState(virtualMachine, &powerState) < 0)) { + + /* Raise error from last lookup if we didn't successfuly fetch any + * domain objecst yet */
typo.
Also, the comment does not address the fact that we only raise error if there are no more domains to fetch.
Yeah I utterly failed at trying to express that concisely and gave up I guess. :D Perhaps: "Raise error only if we didn't successfuly fill any domain" ?
participants (2)
-
Ján Tomko -
Peter Krempa