Stephen Gallagher <sgallagh(a)redhat.com> wrote:
On 08/31/2010 07:57 AM, Jan Zelený wrote:
> Stephen Gallagher <sgallagh(a)redhat.com> wrote:
>>>> 0005-Deleted-some-needless-assignments.patch
>>>> Ticket: #582
>>>
>>> I think we should check for potential errors from sdap_id_op_done()
>>> calls instead of removing the assignments altogether.
>>
>> I agree with Jakub. These assignments shouldn't be removed, they should
>> be tested for failures.
>
> Sorry for an extra email, forgot to mention this in the previous one. I
> don't think this is the case. The result of operation is subsequently
> checked (it is stored also in dp_error variable). We can check ret value
> as well as dp_error, but I don't think it will bring any code
> improvement. The same applies for call of sysdb_attrs_get_string() - its
> return value isn't checked, but the content of "value" variable is
> tested.
For the record, clang and Coverity both throw warnings if we do not
receive and check the return value of any function that has a return
value. In order to be in compliance with these tools, we need to handle
this.
Also, our unofficial policy is that the return value needs to be
accurate (since the returned-by-reference value may not be valid or safe
to dereference if the return value is a failure).
Both your points are valid. I'm attaching a patch where the check is
performed. I still think it is redundant, but it might be more safe in the
future in case the code around it changes.
--
Jan