2018-07-03 11:31 GMT+02:00 Michal Prívozník <mprivozn@xxxxxxxxxx>:
> 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@xxxxxxxxx>
>> ---
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
