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

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.