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. -- Matthias Bolte http://photron.blogspot.com