On (11/01/16 14:07), Pavel Březina wrote:
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.
Which responder would print following message if the aofs back end
is not configured?
And try to look to other cases as well :-)
1779 if (!be_cli->bectx->bet_info[BET_AUTOFS].bet_ops) {
1780 DEBUG(SSSDBG_CRIT_FAILURE, "Undefined backend target.\n");
1781 be_sbus_reply_data_set(&req_reply, DP_ERR_FATAL, ENODEV,
1782 "Autofs back end target is not
configured");
1783 goto done;
1784 }
LS