On (19/02/15 10:31), Pavel Reichl wrote:
On 02/18/2015 10:18 PM, Lukas Slebodnik wrote:
>On (18/02/15 18:32), Pavel Reichl wrote:
>>On 01/27/2015 09:02 AM, Lukas Slebodnik wrote:
>>>On (26/01/15 20:14), Sumit Bose wrote:
>>>>I think there is still a chance that has_posix is uninitialised.
>>>>After ret != EOK is returned in 325 we jump in the if-block starting at
>>>>331. If sdap_id_op_done() finishes successful has_posix is still
>>>>undefined but ret == EOK in line 346.
>>>>
>>>>325 ret = sdap_posix_check_recv(subreq, &has_posix);
>>>>326 talloc_zfree(subreq);
>>>>327 if (ret != EOK) {
>>>>328 /* We can only finish the id_op on error as the connection
>>>>329 * is re-used by the user search
>>>>330 */
>>>>331 ret = sdap_id_op_done(state->op, ret, &dp_error);
>>>>332 if (dp_error == DP_ERR_OK && ret != EOK) {
>>>>333 /* retry */
>>>>334 ret = users_get_retry(req);
>>>>335 if (ret != EOK) {
>>>>336 tevent_req_error(req, ret);
>>>>337 }
>>>>338 return;
>>>>339 }
>>>>340 }
>>>>341
>>>>342 state->ctx->srv_opts->posix_checked = true;
>>>>343
>>>>344 /* If the check ran to completion, we know for certain about the
attributes
>>>>345 */
>>>>346 if (has_posix == false) {
>>>>347 state->sdap_ret = ERR_NO_POSIX;
>>>>348 tevent_req_done(req);
>>>>349 return;
>>>>350 }
>>>>
>>>You are right.
>>>The variable ret is used for checking 3 functions sdap_posix_check_recv,
>>>sdap_id_op_done, users_get_retry.
>>>
>>>I decided to create another variable.
>>>
>>>The other alternative is to initialize boolean variable has_posix to true,
>>>becuase if there is any problem with connection we should still expect there
>>>can be posix attributes.
>>>
>>>LS
>>>
>>Not nice, but I believe it address original problem and Sumit's comments
>>comments as well.
>>
>>So ACK by me.
>>
>If you prefer to initialize boolean variable has_posix to true
>I can do it. (The patch will be simpler.)
>
>LS
Thanks for option but I don't like it any better. I think it's not a good
programming practise to expect anything about output parameter (has_posix) of
function in case it fails. I trust you it would work in this case, but
generally....
I had the same feeling.
I don't see any easy way how to provide a better fix for the bug,
so I repeat
- ACK to your patch.
Thank you for review.
LS