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