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

[Michal Prívozník]

On 07/03/2018 03:43 PM, Matthias Bolte wrote:
> 2018-07-03 11:31 GMT+02:00 Michal Prívozník <mprivozn at 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 at 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