On Mon, Jan 11, 2016 at 02:07:22PM +0100, 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.
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);
}
What about:
diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
index 41680c7..a653acd 100644
--- a/src/providers/data_provider_be.c
+++ b/src/providers/data_provider_be.c
@@ -272,7 +272,9 @@ static errno_t be_sbus_reply(struct sbus_request *sbus_req,
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");
+ DEBUG(SSSDBG_TRACE_LIBS,
+ "Cannot handle request: %s",
+ err_msg ? err_msg : "Handler not configured\n");
} else {
DEBUG(SSSDBG_TRACE_LIBS,
"Request processed. Returned [%s]:%d,%d,%s\n",