On Mon, Jan 11, 2016 at 02:58:59PM +0100, Lukas Slebodnik wrote:
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?
I'll send that patch so we can review it properly with CI and stuff..