Re: [libvirt] [PATCH 1/3] esx_vi.c: Simplify error handling in MachineByName

2018-07-03 11:31 GMT+02:00 Michal Prívozník <mprivozn(a)redhat.com&gt;: > 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(a)gmail.com&gt; >> --- >> 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.