On 07/03/2018 03:43 PM, Matthias Bolte wrote:
> 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.
>
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
