On (11/01/16 14:19), Jakub Hrozek wrote:
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",
I'm fine
with such change.
Any other objections?
LS