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

[Matthias Bolte]

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.

-- 
Matthias Bolte
http://photron.blogspot.com