----- Original Message ----- > From: "Jakub Hrozek" <jhro...@redhat.com> > To: sssd-devel@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. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
