[SSSD] [PATCH] Convert the simple access check to new error codes

Pavel Březina pbrezina at redhat.com
Fri Apr 19 12:25:19 UTC 2013


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.




More information about the sssd-devel mailing list