-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 04/19/2013 08:25 AM, Pavel Březina wrote:
On 04/18/2013 01:44 PM, Jakub Hrozek wrote:
> On Thu, Apr 18, 2013 at 10:59:11AM +0200, Pavel Březina wrote:
>>> On 04/17/2013 11:39 AM, Jakub Hrozek wrote:
>>>>> Hi,
>>>>>
>>>>> I took a look into converting the simple access provider
>>>>> to use
>>> the new
>>>>> error codes and I think a patch might not even be needed.
>>>>> The
>>> current
>>>>> request returns errno on error and boolean for access
>>> granted/denied.
>>>>>
>>>>> I think the boolean is actually nice in the case and
>>>>> converting to ERR_ACCESS_DENIED instead of false doesn't
>>>>> make much sense.
>>>>>
>>>>> The attached patch just makes the top-level request
>>>>> called by the DP handler return ERR_ACCOUNT_UNKNOWN in
>>>>> case the user passed via
>>> struct
>>>>> pam_data does not exist in sysdb and ERR_INTERNAL instead
>>>>> of EIO for internal errors. It would make the error
>>>>> reporting somewhat nicer, I think, but even if we drop
>>>>> the patch I would like to close
>>>>>
https://fedorahosted.org/sssd/ticket/453
>>>
>>> Nack.
>>>
>>> You need to use sss_strerror() when dealing with new error
>>> codes.
>>>
>>>>> @@ -629,11 +631,15 @@ struct tevent_req
>>> *simple_access_check_send(TALLOC_CTX *mem_ctx,
>>>>> DEBUG(SSSDBG_FUNC_DATA, ("Simple access check for %s\n",
>>> username));
>>>>>
>>>>> ret = simple_check_users(ctx, username,
>>> &state->access_granted);
>>>>> - if (ret != EAGAIN) { - /* Both access denied
>>>>> and an error */ + if (ret != EOK && ret != EAGAIN) { +
>>>>> ret = ERR_INTERNAL; + goto immediate; + } else
>>>>> if (ret == EOK) { goto immediate; }
>>>
>>> How about if (ret == EOK) { goto immediate; } else if (ret !=
>>> EAGAIN) { ret = ERR_INTERNAL; goto immediate; }
> Thank you, a new patch is attached.
> subreq = simple_check_get_groups_send(state, ev, ctx, username);
> if (!subreq) { - ret = EIO; + ret = ERR_INTERNAL;
> goto immediate; }
Hi, I think we should stick with ENOMEM for tevent request - when
_send returns NULL it pretty much means that it couldn't create
request because some malloc failed. But if you want to use other
code for it, I'd rather use something else than ERR_INTERNAL.
I agree. If simple_check_get_groups_send() can fail for any reason
other than ENOMEM, it should be doing so in a tevent_req_immediate()
failure handler, not returning NULL for the subreq. If it's doing
that, it's a style violation.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (GNU/Linux)
Comment: Using GnuPG with Thunderbird -
http://www.enigmail.net/
iEYEARECAAYFAlFxRQMACgkQeiVVYja6o6PgtwCgmROFkxykuhyY41Q5DJwVEKOs
E0YAni3Jqvs3Yo26zj26s/7ynlosc1kI
=qK2o
-----END PGP SIGNATURE-----