On Wed, May 11, 2016 at 10:28:16AM +0200, Jakub Hrozek wrote:
> On Tue, May 10, 2016 at 12:53:08PM +0200, Pavel Březina wrote:
> > On 05/10/2016 12:34 PM, Jakub Hrozek wrote:
> > > 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.
> >
> >
> > Ack.
>
> Thank you for the careful review. Pushed to master:
> e8474ac0be7e81c0ca54eb09e2fef42595602945
> c30b7a1931211fdcae0564551a7625cc4f6dee9f
> and sssd-1-13:
> 7ff6858b18fb463bc446797aa860960d5165fe9e
> 406a7e5b731ae79084dce00021e01ebe7b7d724a
Sorry, I forgot the CI link:
http://sssd-ci.duckdns.org/logs/job/43/11/summary.html
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
