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<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.
Ack.