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
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; }
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.
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.
-----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.
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.
----- Original Message -----
From: "Jakub Hrozek" jhrozek@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.
On Fri, Apr 19, 2013 at 11:22:56AM -0400, Pavel Brezina wrote:
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.
Pushed to master.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Pavel Březina -
Stephen Gallagher