On Tue, May 10, 2016 at 12:06:39PM +0200, Pavel Březina wrote:
> On 05/05/2016 11:38 AM, Jakub Hrozek wrote:
> > On Wed, Apr 27, 2016 at 11:47:50AM +0200, Pavel Březina wrote:
> > > >Can you also extend sbus_request_invoke_or_finish() to treat
> > > >ERR_SBUS_REQUEST_HANDLED the same way as EOK? I.e.
> > > >
> > > >case EOK:
> > > >case ERR_SBUS_REQUEST_HANDLED:
> > > > return;
> > > >
> > > >This way you don't have to translate the new error code back to EOK.
> > Sorry, I totally forgot about these patches.
> >
> > Here you go..
> >
> >
> > 0001-UTIL-Add-ERR_SBUS_REQUEST_HANDLED.patch
> >
> >
> > From d3b578dd84acd327f0f623ddb835cd031480bb0a Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek<jhro...@redhat.com>
> > Date: Wed, 27 Apr 2016 11:11:31 +0200
> > Subject: [PATCH 1/2] UTIL: Add ERR_SBUS_REQUEST_HANDLED
> >
> > In most cases when sbus request parsing finishes, the request is handled
> > internally and a reply is sent to the caller. However, in handlers that
> > are parsed and handled completely manually, we might want to be notified
> > about this case so that the called of sbus_request_parse_or_finish()
> > aborts the request and doesn't proceed with using the sbus request which
> > is already freed internally in sbus_request_parse_or_finish().
> > ---
> > src/util/util_errors.c | 1 +
> > src/util/util_errors.h | 1 +
> > 2 files changed, 2 insertions(+)
> >
> > diff --git a/src/util/util_errors.c b/src/util/util_errors.c
> > index
> > 59ae63ab8d6e834a772349b162bf282f9a4f1c72..c998e14c26e43c3cd6a5a060bb6f74698b9e93ae
> > 100644
> > --- a/src/util/util_errors.c
> > +++ b/src/util/util_errors.c
> > @@ -84,6 +84,7 @@ struct err_string error_to_str[] = {
> > { "Subdomain is inactive" }, /* ERR_SUBDOM_INACTIVE */
> > { "Account is locked" }, /* ERR_ACCOUNT_LOCKED */
> > { "AD renewal child failed" }, /* ERR_RENEWAL_CHILD */
> > + { "SBUS request already handled" }, /* ERR_SBUS_REQUEST_HANDLED */
> > { "ERR_LAST" } /* ERR_LAST */
> > };
> >
> > diff --git a/src/util/util_errors.h b/src/util/util_errors.h
> > index
> > 05791f2f08f107a8b4830b810b8826983763174f..c0d9622a431a9946fdfa5e5c60ecf7b9e1ae66a5
> > 100644
> > --- a/src/util/util_errors.h
> > +++ b/src/util/util_errors.h
> > @@ -106,6 +106,7 @@ enum sssd_errors {
> > ERR_SUBDOM_INACTIVE,
> > ERR_ACCOUNT_LOCKED,
> > ERR_RENEWAL_CHILD,
> > + ERR_SBUS_REQUEST_HANDLED,
> > ERR_LAST /* ALWAYS LAST */
> > };
> >
> > -- 2.4.11
> >
> >
> > 0002-IFP-Do-not-crash-on-invalid-arguments-to-GetUserAttr.patch
> >
> >
> > From 567b8dc9bae4f5a83bacfccc310376ffc27d7ae8 Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek<jhro...@redhat.com>
> > Date: Wed, 20 Apr 2016 11:54:31 +0200
> > Subject: [PATCH 2/2] IFP: Do not crash on invalid arguments to GetUserAttr
> >
> > ---
> > src/responder/ifp/ifpsrv_cmd.c | 8 +++++---
> > src/sbus/sssd_dbus_request.c | 1 +
> > 2 files changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/responder/ifp/ifpsrv_cmd.c b/src/responder/ifp/ifpsrv_cmd.c
> > index
> > 399e83e0255700df5a24491a3eb33b4f92040ca7..b1e8f80f22d535b5e2579ac25b39b61b0fa5b33b
> > 100644
> > --- a/src/responder/ifp/ifpsrv_cmd.c
> > +++ b/src/responder/ifp/ifpsrv_cmd.c
> > @@ -82,8 +82,10 @@ int ifp_user_get_attr(struct sbus_request *dbus_req,
> > void *data)
> > attr_req->ireq = ireq;
> >
> > ret = ifp_user_get_attr_unpack_msg(attr_req);
> > - if (ret != EOK) {
> > - return ret; /* handled internally */
> > + if (ret == ERR_SBUS_REQUEST_HANDLED) {
> > + return EOK; /* handled internally */
> > + } else if (ret != EOK) {
> > + return ret; /* internal error */
> > }
>
> You can remove this hunk and stick with:
> if (ret != EOK) {
> return ret;
> }
>
> Since ERR_SBUS_REQUEST_HANDLED is now returned and another reply will not be
> sent thanks to following hunks.
>
> >
> > DEBUG(SSSDBG_FUNC_DATA,
> > @@ -117,7 +119,7 @@ ifp_user_get_attr_unpack_msg(struct ifp_attr_req
> > *attr_req)
> > DBUS_TYPE_INVALID);
> > if (parsed == false) {
> > DEBUG(SSSDBG_OP_FAILURE, "Could not parse arguments\n");
> > - return EOK; /* handled */
> > + return ERR_SBUS_REQUEST_HANDLED;
> > }
> >
> > /* Copy the attributes to maintain memory hierarchy with talloc */
> > diff --git a/src/sbus/sssd_dbus_request.c b/src/sbus/sssd_dbus_request.c
> > index
> > aa57f6b6587183a9edd7764d123e82b01b5f6070..c71a79b1f06c92c25f8bb836b5bf815c056d3912
> > 100644
> > --- a/src/sbus/sssd_dbus_request.c
> > +++ b/src/sbus/sssd_dbus_request.c
> > @@ -74,6 +74,7 @@ sbus_request_invoke_or_finish(struct sbus_request
> > *dbus_req,
> > }
> >
> > switch(ret) {
> > + case ERR_SBUS_REQUEST_HANDLED:
>
> I think this should be in a separate patch, or squashed to the first one.
Thanks both done. I don't think we need a separate one-liner.
>From 5e14e17d96e365d3a507adc4bbd3b7e634ba2d69 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Wed, 27 Apr 2016 11:11:31 +0200
Subject: [PATCH 1/2] UTIL: Add ERR_SBUS_REQUEST_HANDLED
In most cases when sbus request parsing finishes, the request is handled
internally and a reply is sent to the caller. However, in handlers that
are parsed and handled completely manually, we might want to be notified
about this case so that the called of sbus_request_parse_or_finish()
aborts the request and doesn't proceed with using the sbus request which
is already freed internally in sbus_request_parse_or_finish().
---
src/sbus/sssd_dbus_request.c | 1 +
src/util/util_errors.c | 1 +
src/util/util_errors.h | 1 +
3 files changed, 3 insertions(+)
diff --git a/src/sbus/sssd_dbus_request.c b/src/sbus/sssd_dbus_request.c
index
aa57f6b6587183a9edd7764d123e82b01b5f6070..c71a79b1f06c92c25f8bb836b5bf815c056d3912
100644
--- a/src/sbus/sssd_dbus_request.c
+++ b/src/sbus/sssd_dbus_request.c
@@ -74,6 +74,7 @@ sbus_request_invoke_or_finish(struct sbus_request *dbus_req,
}
switch(ret) {
+ case ERR_SBUS_REQUEST_HANDLED:
case EOK:
return;
case ENOMEM:
diff --git a/src/util/util_errors.c b/src/util/util_errors.c
index
59ae63ab8d6e834a772349b162bf282f9a4f1c72..c998e14c26e43c3cd6a5a060bb6f74698b9e93ae
100644
--- a/src/util/util_errors.c
+++ b/src/util/util_errors.c
@@ -84,6 +84,7 @@ struct err_string error_to_str[] = {
{ "Subdomain is inactive" }, /* ERR_SUBDOM_INACTIVE */
{ "Account is locked" }, /* ERR_ACCOUNT_LOCKED */
{ "AD renewal child failed" }, /* ERR_RENEWAL_CHILD */
+ { "SBUS request already handled" }, /* ERR_SBUS_REQUEST_HANDLED */
{ "ERR_LAST" } /* ERR_LAST */
};
diff --git a/src/util/util_errors.h b/src/util/util_errors.h
index
05791f2f08f107a8b4830b810b8826983763174f..c0d9622a431a9946fdfa5e5c60ecf7b9e1ae66a5
100644
--- a/src/util/util_errors.h
+++ b/src/util/util_errors.h
@@ -106,6 +106,7 @@ enum sssd_errors {
ERR_SUBDOM_INACTIVE,
ERR_ACCOUNT_LOCKED,
ERR_RENEWAL_CHILD,
+ ERR_SBUS_REQUEST_HANDLED,
ERR_LAST /* ALWAYS LAST */
};
--
2.4.11
>From dfc33ddcfd41b0d44471acdd7395356273b54d0f Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Tue, 10 May 2016 12:24:44 +0200
Subject: [PATCH 2/2] IFP: Do not crash on invalid arguments to GetUserAttr
---
src/responder/ifp/ifpsrv_cmd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/responder/ifp/ifpsrv_cmd.c b/src/responder/ifp/ifpsrv_cmd.c
index
399e83e0255700df5a24491a3eb33b4f92040ca7..2c0ceb817154b7cb60d2d65637bfebd3e738992f
100644
--- a/src/responder/ifp/ifpsrv_cmd.c
+++ b/src/responder/ifp/ifpsrv_cmd.c
@@ -83,7 +83,7 @@ int ifp_user_get_attr(struct sbus_request *dbus_req, void
*data)
ret = ifp_user_get_attr_unpack_msg(attr_req);
if (ret != EOK) {
- return ret; /* handled internally */
+ return ret; /* internal error */
}
DEBUG(SSSDBG_FUNC_DATA,
@@ -117,7 +117,7 @@ ifp_user_get_attr_unpack_msg(struct ifp_attr_req *attr_req)
DBUS_TYPE_INVALID);
if (parsed == false) {
DEBUG(SSSDBG_OP_FAILURE, "Could not parse arguments\n");
- return EOK; /* handled */
+ return ERR_SBUS_REQUEST_HANDLED;
}
/* Copy the attributes to maintain memory hierarchy with talloc */
--
2.4.11
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
