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

Reply via email to