From: "Jakub Hrozek" <jhrozek(a)redhat.com>
To: sssd-devel(a)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.