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

Pavel Brezina pbrezina at redhat.com
Fri Apr 19 15:22:56 UTC 2013


----- Original Message -----
> From: "Jakub Hrozek" <jhrozek at redhat.com>
> To: sssd-devel at lists.fedorahosted.org
> Sent: Friday, April 19, 2013 3:54:34 PM
> Subject: Re: [SSSD] [PATCH] Convert the simple access check to new error	codes
> 
> On Fri, Apr 19, 2013 at 09:22:11AM -0400, Stephen Gallagher wrote:
> > -----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.
> 
> This is true for any new code, many old requests still blindly return
> NULL on any error.
> 
> That doesn't make Pavel's recommendation any less true, so a new patch
> is attached.

Ack.



More information about the sssd-devel mailing list