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<jhrozek(a)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<jhrozek(a)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.