On 07/03/2018 03:43 PM, Matthias Bolte wrote:
2018-07-03 11:31 GMT+02:00 Michal Prívozník <mprivozn@redhat.com>:
On 07/03/2018 04:20 AM, Marcos Paulo de Souza wrote:
The same pattern is used in lots of other places.
Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com> --- src/esx/esx_vi.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index 43ff7ea048..25fbdc7e44 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -3014,16 +3014,10 @@ esxVI_LookupVirtualMachineByName(esxVI_Context *ctx, const char *name, break; }
- if (!(*virtualMachine)) { - if (occurrence == esxVI_Occurrence_OptionalItem) { - result = 0; - - goto cleanup; - } else { - virReportError(VIR_ERR_NO_DOMAIN, - _("Could not find domain with name '%s'"), name); - goto cleanup; - } + if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem) { + virReportError(VIR_ERR_NO_DOMAIN, + _("Could not find domain with name '%s'"), name); + goto cleanup; }
I think this error message should be dropped. Firstly, this function is called from esxDomainDefineXMLFlags() where it is used to check if a domain with the same name as in provided XML does not exist. If it doesn't this function reports an error which is undesired.
Secondly, every caller checks for virtualMachine == NULL and reports their own error effectively overwriting this one.
Thirdly, this function is supposed to act like virDomainObjListFindByName() which doesn't report error either.
ACK with this squashed in:
diff --git i/src/esx/esx_vi.c w/src/esx/esx_vi.c index 25fbdc7e44..0bdfc5a8be 100644 --- i/src/esx/esx_vi.c +++ w/src/esx/esx_vi.c @@ -3014,11 +3014,8 @@ esxVI_LookupVirtualMachineByName(esxVI_Context *ctx, const char *name, break; }
- if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem) { - virReportError(VIR_ERR_NO_DOMAIN, - _("Could not find domain with name '%s'"), name); + if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem) goto cleanup; - }
result = 0;
And pushed.
The original patch was fine, your modification is wrong. The error message needs to stay. You didn't follow the logic closely enough. The caller of esxVI_LookupVirtualMachineByName specified the expected occurrence.
esxDomainDefineXMLFlags uses this to test if the domain exists and calls esxVI_LookupVirtualMachineByName with occurrence OptionalItem. If the domain is missing no error is reported by esxVI_LookupVirtualMachineByName, because of occurrence OptionalItem.
esxDomainLookupByName uses this to find a domain with the given name. It also uses occurrence OptionalItem. Here you argue that the caller checks for virtualMachine == NULL anyway. But no error is overwritten here because esxVI_LookupVirtualMachineByName doesn't report an error with occurrence OptionalItem. The actual point that could be made here is that esxDomainLookupByName should actually use occurrence RequiredItem instead of doing the virtualMachine == NULL check itself.
Also esxVI_LookupVirtualMachineByUuid has the same logic that got simplified in the original patch, but again, the error reporting there is correct as well and needs to stay.
D'oh. Well, so far all (as in both) callers pass OptionalItem so no real harm done. But sure, I'll post a patch to fix my mess. Michal