On 01/08/2016 06:16 PM, Lukas Slebodnik wrote:
On (10/12/15 10:59), Jakub Hrozek wrote:
> On Wed, Dec 09, 2015 at 01:10:58PM +0100, Lukas Slebodnik wrote:
>> On (04/12/15 16:42), Jakub Hrozek wrote:
>>> On Thu, Dec 03, 2015 at 01:24:54PM +0100, Pavel Březina wrote:
>>>> On 11/20/2015 12:04 PM, Jakub Hrozek wrote:
>>>>> On Thu, Nov 19, 2015 at 01:51:54PM +0100, Pavel Březina wrote:
>>>>>>> Thanks, new patches are attached.
>>>>>>
>>>>>> I had a phone call with Jakub and we decided that it will be
better to use
>>>>>> be_req directly instead of lower level sbus_req. This will allow
us to
>>>>>> simplify it more.
>>>>>
>>>>> OK, see the attached patches..
>>>>
>>>> Hi, thanks for the changes.
>>>>
>>>> First patch:
>
> [...]
>
>>> @@ -199,6 +205,11 @@ struct be_req *be_req_create(TALLOC_CTX *mem_ctx,
>>> be_req->domain = be_ctx->domain;
>>> be_req->fn = fn;
>>> be_req->pvt = pvt_fn_data;
>>> + be_req->req_name = talloc_strdup(be_req, name);
>>> + if (be_req->req_name == NULL) {
>>> + talloc_free(be_req);
>>> + return NULL;
>>> + }
>>>
>>> /* Add this request to active request list and make sure it is
>>> * removed on termination. */
>>> @@ -242,6 +253,95 @@ void be_req_terminate(struct be_req *be_req,
>>> be_req->fn(be_req, dp_err_type, errnum, errstr);
>>> }
>>>
>>> +
>> I know it's a nitpick, but you added extra new line here :-)
>> and you should change 2nd patch anyway.
>
> I removed the extra line.
>
>> >From d7b00e1ef4984bb17d4ce62b2c206efebb6aea37 Mon Sep 17 00:00:00 2001
>>> From: Jakub Hrozek <jhrozek(a)redhat.com>
>>> Date: Wed, 11 Nov 2015 13:40:16 +0100
>>> Subject: [PATCH 2/2] DP: Reduce code duplication in Data Provider handlers
>>>
>>> Instead of setting the three same variables over again, add a structure
>>> be_sbus_reply_data with a default initializer BE_SBUS_REPLY_DATA_INIT.
>>>
>>> The handlers can then set the structure to BE_SBUS_REPLY_DATA_INIT on
>>> declaration or set a particular value with be_sbus_reply_data_set.
>>>
>>> The handler can also reply to the message (typically on failure state)
>>> with be_sbus_req_reply_data()
>
> [...]
>
>>> @@ -791,9 +821,8 @@ static int be_get_subdomains(struct sbus_request
*dbus_req, void *user_data)
>>> */
>>> if (becli->bectx->offstat.offline) {
>>> DEBUG(SSSDBG_TRACE_FUNC, "Cannot proceed, provider is
offline.\n");
>> I wanted to push patches but I moticed that DEBUG message was removed
>> in previous case "Undefined backend target", but was not removed here.
>> If we want to remove them, please remove them on all places.
>> If we do not want to touch them, do not remove it in previous case.
>
> I left the original DEBUG message. I think all the unspecified target
> messages could use some love at least to standardize on the same levels,
> but we have a dedicated ticket for that..
>From 5c6d0151b9a9d7667fe3d24005b2c222bf6f6003 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek <jhrozek(a)redhat.com>
> Date: Wed, 11 Nov 2015 13:39:43 +0100
> Subject: [PATCH 1/2] DP: Reduce code duplication in the callback handlers
>
> Instead of calling sbus_request_return_and_finish() directly with the
> same checks copied over, add a be_sbus_reply() helper instead.
> ---
> src/providers/ad/ad_subdomains.c | 2 +-
> src/providers/data_provider_be.c | 354 +++++++++++++------------------------
> src/providers/dp_backend.h | 11 +-
> src/providers/ipa/ipa_subdomains.c | 2 +-
> 4 files changed, 137 insertions(+), 232 deletions(-)
>
> diff --git a/src/providers/ad/ad_subdomains.c b/src/providers/ad/ad_subdomains.c
> index
2e5d9120e473e32a84610d607ccf329249b4ac9e..4799b518a1354e5b4ef8392b860effc9121ee121 100644
> --- a/src/providers/ad/ad_subdomains.c
> +++ b/src/providers/ad/ad_subdomains.c
> @@ -1118,7 +1118,7 @@ static void ad_subdom_online_cb(void *pvt)
>
> refresh_interval = ctx->be_ctx->domain->subdomain_refresh_interval;
>
> - be_req = be_req_create(ctx, NULL, ctx->be_ctx,
> + be_req = be_req_create(ctx, NULL, ctx->be_ctx, "AD subdomains",
> ad_subdom_be_req_callback, NULL);
> if (be_req == NULL) {
> DEBUG(SSSDBG_CRIT_FAILURE, "be_req_create() failed.\n");
> diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
> index
9bc54b6d59dafd22b9f70da7cca3b520ca941efc..f7f096b732c8168a508e086008f7895248cbe55b 100644
> --- a/src/providers/data_provider_be.c
> +++ b/src/providers/data_provider_be.c
> @@ -174,6 +174,9 @@ struct be_req {
> */
> int phase;
>
> + /* Just for nicer debugging */
> + const char *req_name;
> +
> struct be_req *prev;
> struct be_req *next;
> };
> @@ -186,8 +189,11 @@ static int be_req_destructor(struct be_req *be_req)
> }
>
> struct be_req *be_req_create(TALLOC_CTX *mem_ctx,
> - struct be_client *becli, struct be_ctx *be_ctx,
> - be_async_callback_t fn, void *pvt_fn_data)
> + struct be_client *becli,
> + struct be_ctx *be_ctx,
> + const char *name,
> + be_async_callback_t fn,
> + void *pvt_fn_data)
> {
> struct be_req *be_req;
>
> @@ -199,6 +205,11 @@ struct be_req *be_req_create(TALLOC_CTX *mem_ctx,
> be_req->domain = be_ctx->domain;
> be_req->fn = fn;
> be_req->pvt = pvt_fn_data;
> + be_req->req_name = talloc_strdup(be_req, name);
> + if (be_req->req_name == NULL) {
> + talloc_free(be_req);
> + return NULL;
> + }
>
> /* Add this request to active request list and make sure it is
> * removed on termination. */
> @@ -242,6 +253,94 @@ void be_req_terminate(struct be_req *be_req,
> be_req->fn(be_req, dp_err_type, errnum, errstr);
> }
>
> +static errno_t be_sbus_reply(struct sbus_request *sbus_req,
> + dbus_uint16_t err_maj,
> + dbus_uint32_t err_min,
> + const char *err_msg)
> +{
> + errno_t ret;
> + const char *safe_err_msg;
> +
> + /* Only return a reply if one was requested
> + * There may not be one if this request began
> + * while we were offline
> + */
> + if (sbus_req == NULL) {
> + return EOK;
> + }
> +
> + safe_err_msg = safe_be_req_err_msg(err_msg, err_maj);
> +
> + if (err_maj == DP_ERR_FATAL && err_min == ENODEV) {
> + DEBUG(SSSDBG_TRACE_LIBS, "Handler not configured\n");
"err_msg" is not printed in case of
"err_maj == DP_ERR_FATAL && err_min == ENODEV"
>From 33a8b116c50f19af6c90c4a899f747b095a253ef Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek <jhrozek(a)redhat.com>
> Date: Wed, 11 Nov 2015 13:40:16 +0100
> Subject: [PATCH 2/2] DP: Reduce code duplication in Data Provider handlers
>
> Instead of setting the three same variables over again, add a structure
> be_sbus_reply_data with a default initializer BE_SBUS_REPLY_DATA_INIT.
>
> The handlers can then set the structure to BE_SBUS_REPLY_DATA_INIT on
> declaration or set a particular value with be_sbus_reply_data_set.
>
> The handler can also reply to the message (typically on failure state)
> with be_sbus_req_reply_data()
> ---
> src/providers/data_provider_be.c | 269 +++++++++++++++------------------------
> 1 file changed, 102 insertions(+), 167 deletions(-)
>
> diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
> index
f7f096b732c8168a508e086008f7895248cbe55b..8e2baf27dd260b7d9197da8bfa6db2dad767de89 100644
> --- a/src/providers/data_provider_be.c
> +++ b/src/providers/data_provider_be.c
> @@ -341,6 +341,40 @@ static errno_t be_offline_reply(struct sbus_request
**sbus_req_ptr)
> return ret;
> }
>
> +struct be_sbus_reply_data {
> + dbus_uint16_t err_maj;
> + dbus_uint32_t err_min;
> + const char *err_msg;
> +};
> +
> +#define BE_SBUS_REPLY_DATA_INIT { .err_maj = DP_ERR_FATAL, \
> + .err_min = ERR_INTERNAL, \
> + .err_msg = "Fatal error" \
> + };
> +
> +static inline void be_sbus_reply_data_set(struct be_sbus_reply_data *rdata,
> + dbus_uint16_t err_maj,
> + dbus_uint32_t err_min,
> + const char *err_msg)
> +{
> + if (rdata == NULL) {
> + DEBUG(SSSDBG_CRIT_FAILURE,
> + "Bug: Attempt to set NULL be_sbus_reply_data\n");
> + return;
> + }
> +
> + rdata->err_maj = err_maj;
> + rdata->err_min = err_min;
> + rdata->err_msg = err_msg;
> +}
> +
> +static inline errno_t be_sbus_req_reply_data(struct sbus_request *sbus_req,
> + struct be_sbus_reply_data *data)
> +{
> + return be_sbus_reply(sbus_req, data->err_maj,
> + data->err_min, data->err_msg);
> +}
> +
> void be_terminate_domain_requests(struct be_ctx *be_ctx,
> const char *domain)
> {
> @@ -761,9 +795,7 @@ static int be_get_subdomains(struct sbus_request *dbus_req, void
*user_data)
> struct be_req *be_req = NULL;
> struct be_client *becli;
> char *domain_hint;
> - dbus_uint16_t err_maj;
> - dbus_uint32_t err_min;
> - const char *err_msg;
> + struct be_sbus_reply_data req_reply = BE_SBUS_REPLY_DATA_INIT;
> int ret;
>
> becli = talloc_get_type(user_data, struct be_client);
> @@ -777,9 +809,8 @@ static int be_get_subdomains(struct sbus_request *dbus_req, void
*user_data)
> /* return an error if corresponding backend target is not configured */
> if (becli->bectx->bet_info[BET_SUBDOMAINS].bet_ops == NULL) {
> DEBUG(SSSDBG_TRACE_INTERNAL, "Undefined backend target.\n");
> - err_maj = DP_ERR_FATAL;
> - err_min = ENODEV;
> - err_msg = "Subdomains back end target is not configured";
> + be_sbus_reply_data_set(&req_reply, DP_ERR_FATAL, ENODEV,
> + "Subdomains back end target is not
configured");
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This error message will not be printed
becuase of DP_ERR_FATAL, ENODEV
Do we want to change it. So error message will be printed as well?
The same situation is also on ther places in the second patch.
I know that patches were pushed :-)
LS
It should be printed on responder side. But maybe the following true
branch to also print message.
if (err_maj == DP_ERR_FATAL && err_min == ENODEV) {
DEBUG(SSSDBG_TRACE_LIBS, "Handler not configured\n");
} else {
DEBUG(SSSDBG_TRACE_LIBS,
"Request processed. Returned [%s]:%d,%d,%s\n",
dp_err_to_string(err_maj), err_maj, err_min, err_msg);
}