Hi,
I think one of the prime reasons for #2861 was copy-pasting code. The two attached patches reduce the code duplication and hopefully will make future additions to Data Provider safer.
Ideas on different solutions are welcome!
On 11/11/2015 02:28 PM, Jakub Hrozek wrote:
Hi,
I think one of the prime reasons for #2861 was copy-pasting code. The two attached patches reduce the code duplication and hopefully will make future additions to Data Provider safer.
Ideas on different solutions are welcome!
Hi Jakub,
I am afraid that the first patch is inapplicable. It is build up on 562a15a2d156b4b062acbf1f4e44e4cb7a4058d2 commit but there is no such commit.
Regards
Petr
0001-DP-Reduce-code-duplication-in-the-callback-handlers.patch
From f6e929d4132a23d23a9016e43f4b870780c53032 Mon Sep 17 00:00:00 2001 From: Jakub Hrozekjhrozek@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.
pcech@albireo ~/sssd: (master) $ git am ../patch/0001-DP-Reduce-code-duplication-in-the-callback-handlers.patch Applying: DP: Reduce code duplication in the callback handlers error: patch failed: src/providers/data_provider_be.c:661 error: src/providers/data_provider_be.c: patch does not apply Patch failed at 0001 DP: Reduce code duplication in the callback handlers The copy of the patch that failed is found in: /home/pcech/sssd/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". pcech@albireo ~/sssd: (master) $ git am --abort pcech@albireo ~/sssd: (master) $ git am -i3 ../patch/0001-DP-Reduce-code-duplication-in-the-callback-handlers.patch Commit Body is: -------------------------- 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. -------------------------- Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all a Applying: DP: Reduce code duplication in the callback handlers error: invalid object 100644 562a15a2d156b4b062acbf1f4e44e4cb7a4058d2 for 'src/providers/data_provider_be.c' fatal: git-write-tree: error building trees Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 DP: Reduce code duplication in the callback handlers The copy of the patch that failed is found in: /home/pcech/sssd/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
On Thu, Nov 12, 2015 at 01:03:33PM +0100, Petr Cech wrote:
On 11/11/2015 02:28 PM, Jakub Hrozek wrote:
Hi,
I think one of the prime reasons for #2861 was copy-pasting code. The two attached patches reduce the code duplication and hopefully will make future additions to Data Provider safer.
Ideas on different solutions are welcome!
Hi Jakub,
I am afraid that the first patch is inapplicable. It is build up on 562a15a2d156b4b062acbf1f4e44e4cb7a4058d2 commit but there is no such commit.
Regards
Ah, I'm sorry I should have said earlier that the patches must be applied atop the patches in thread called "[PATCH] Guard against invalid DP messages".
These code refactoring patches are not that important, we can wait with review until the other thread is pushed.
On 11/12/2015 01:08 PM, Jakub Hrozek wrote:
On Thu, Nov 12, 2015 at 01:03:33PM +0100, Petr Cech wrote:
On 11/11/2015 02:28 PM, Jakub Hrozek wrote:
Hi,
I think one of the prime reasons for #2861 was copy-pasting code. The two attached patches reduce the code duplication and hopefully will make future additions to Data Provider safer.
Ideas on different solutions are welcome!
Hi Jakub,
I am afraid that the first patch is inapplicable. It is build up on 562a15a2d156b4b062acbf1f4e44e4cb7a4058d2 commit but there is no such commit.
Regards
Ah, I'm sorry I should have said earlier that the patches must be applied atop the patches in thread called "[PATCH] Guard against invalid DP messages".
These code refactoring patches are not that important, we can wait with review until the other thread is pushed.
Well, I looked at thread called "[PATCH] Guard against invalid DP messages". Those patches are still under review. So I will wait for their pushing to the code base.
Please, ping this thread after it.
Thank you
Petr
On Fri, Nov 13, 2015 at 08:29:42AM +0100, Petr Cech wrote:
On 11/12/2015 01:08 PM, Jakub Hrozek wrote:
On Thu, Nov 12, 2015 at 01:03:33PM +0100, Petr Cech wrote:
On 11/11/2015 02:28 PM, Jakub Hrozek wrote:
Hi,
I think one of the prime reasons for #2861 was copy-pasting code. The two attached patches reduce the code duplication and hopefully will make future additions to Data Provider safer.
Ideas on different solutions are welcome!
Hi Jakub,
I am afraid that the first patch is inapplicable. It is build up on 562a15a2d156b4b062acbf1f4e44e4cb7a4058d2 commit but there is no such commit.
Regards
Ah, I'm sorry I should have said earlier that the patches must be applied atop the patches in thread called "[PATCH] Guard against invalid DP messages".
These code refactoring patches are not that important, we can wait with review until the other thread is pushed.
Well, I looked at thread called "[PATCH] Guard against invalid DP messages". Those patches are still under review. So I will wait for their pushing to the code base.
Please, ping this thread after it.
The other thread was pushed.
On 11/11/2015 02:28 PM, Jakub Hrozek wrote:
Hi,
I think one of the prime reasons for #2861 was copy-pasting code. The two attached patches reduce the code duplication and hopefully will make future additions to Data Provider safer.
Ideas on different solutions are welcome!
Hello Jakub,
I see that the previous thread is pushed. So I have started to do review of those patch. Unfortunately the CI tests environment seems to be broken at all, however, local tests passed.
Anyway, I have one little question, look to the second patch.
0001-DP-Reduce-code-duplication-in-the-callback-handlers.patch
From f6e929d4132a23d23a9016e43f4b870780c53032 Mon Sep 17 00:00:00 2001 From: Jakub Hrozekjhrozek@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.
[...]
0002-DP-Reduce-code-duplication-in-Data-Provider-handlers.patch
From caeee4a21bda233f0ec8b08b87a0695029e9af8f Mon Sep 17 00:00:00 2001 From: Jakub Hrozekjhrozek@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 | 266 +++++++++++++++------------------------ 1 file changed, 98 insertions(+), 168 deletions(-)
diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c index eb2f49adce5f5313f31c67b1dfdd21685e69ca3a..de8a8357b8230eddb7f49fff021957c3f580c64e 100644 --- a/src/providers/data_provider_be.c +++ b/src/providers/data_provider_be.c @@ -319,6 +319,36 @@ 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, \
--^-- What does this dot means? It is first time that I see it. Could you explain it to me, please? Is it some kind of syntactic sugar?
Regards
Petr
.err_min = EFAULT, \.err_msg = "Fatal error" \};
On Mon, Nov 16, 2015 at 08:12:06AM +0100, Petr Cech wrote:
+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, \
--^--What does this dot means? It is first time that I see it. Could you explain it to me, please? Is it some kind of syntactic sugar?
Regards
Petr
It's C99 designated initializers: https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
Thank you for reviewing the patch, but it's OK to wait until we fix the CI environment.
On 11/11/2015 02:28 PM, Jakub Hrozek wrote:
Hi,
I think one of the prime reasons for #2861 was copy-pasting code. The two attached patches reduce the code duplication and hopefully will make future additions to Data Provider safer.
Ideas on different solutions are welcome!
+static errno_t be_sbus_req_reply(struct sbus_request *sbus_req,
const char *req_name,int dp_err_type,int errnum,const char *errstr)+{
- const char *req_msg;
- dbus_uint16_t err_maj;
- dbus_uint32_t err_min;
- err_maj = dp_err_type;
- err_min = errnum;
- req_msg = req_name ? req_name : "sbus";
^^ unused, along with req_name
- return be_sbus_reply(sbus_req, err_maj, err_min, errstr);
+}
+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: ...\n"); }
- if (rdata) {
rdata->err_maj = err_maj;rdata->err_min = err_min;rdata->err_msg = err_msg;- }
+}
I like it!
On Mon, Nov 16, 2015 at 11:31:32AM +0100, Pavel Březina wrote:
On 11/11/2015 02:28 PM, Jakub Hrozek wrote:
Hi,
I think one of the prime reasons for #2861 was copy-pasting code. The two attached patches reduce the code duplication and hopefully will make future additions to Data Provider safer.
Ideas on different solutions are welcome!
+static errno_t be_sbus_req_reply(struct sbus_request *sbus_req,
const char *req_name,int dp_err_type,int errnum,const char *errstr)+{
- const char *req_msg;
- dbus_uint16_t err_maj;
- dbus_uint32_t err_min;
- err_maj = dp_err_type;
- err_min = errnum;
- req_msg = req_name ? req_name : "sbus";
^^ unused, along with req_name
Oops, now it is (I couldn't make my mind on how to report the caller, feel free to suggest a better one)
- return be_sbus_reply(sbus_req, err_maj, err_min, errstr);
+}
+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: ...\n");
Done.
}
- if (rdata) {
rdata->err_maj = err_maj;rdata->err_min = err_min;rdata->err_msg = err_msg;- }
+}
I like it!
Thanks, new patches are attached.
On 11/17/2015 11:34 AM, Jakub Hrozek wrote:
On Mon, Nov 16, 2015 at 11:31:32AM +0100, Pavel Březina wrote:
On 11/11/2015 02:28 PM, Jakub Hrozek wrote:
Hi,
I think one of the prime reasons for #2861 was copy-pasting code. The two attached patches reduce the code duplication and hopefully will make future additions to Data Provider safer.
Ideas on different solutions are welcome!
+static errno_t be_sbus_req_reply(struct sbus_request *sbus_req,
const char *req_name,int dp_err_type,int errnum,const char *errstr)+{
- const char *req_msg;
- dbus_uint16_t err_maj;
- dbus_uint32_t err_min;
- err_maj = dp_err_type;
- err_min = errnum;
- req_msg = req_name ? req_name : "sbus";
^^ unused, along with req_nameOops, now it is (I couldn't make my mind on how to report the caller, feel free to suggest a better one)
- return be_sbus_reply(sbus_req, err_maj, err_min, errstr);
+}
+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: ...\n");
Done.
}
- if (rdata) {
rdata->err_maj = err_maj;rdata->err_min = err_min;rdata->err_msg = err_msg;- }
+}
I like it!
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.
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..
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,7 @@ 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 = name;
I know every call now passes static data but IMHO we should be safe and use talloc_strdup here.
+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");- } else {
DEBUG(SSSDBG_TRACE_LIBS,"Request processed. Returned %d,%d,%s\n",err_maj, err_min, err_msg);- }
Can we translate err_maj into string here? To get message similar to: "Request processed: Success [%d]: %s, err_min, err_msg "Request processed: Failure [%d]: %s, err_min, err_msg "Request processed: Offline [%d]: %s, err_min, err_msg
Second patch:
+#define BE_SBUS_REPLY_DATA_INIT { .err_maj = DP_ERR_FATAL, \
.err_min = EFAULT, \.err_msg = "Fatal error" \};
ERR_INTERNAL might be a better choice over EFAULT since it basically means we forgot to set it -> code error.
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,7 @@ 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 = name;
I know every call now passes static data but IMHO we should be safe and use talloc_strdup here.
+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");- } else {
DEBUG(SSSDBG_TRACE_LIBS,"Request processed. Returned %d,%d,%s\n",err_maj, err_min, err_msg);- }
Can we translate err_maj into string here? To get message similar to: "Request processed: Success [%d]: %s, err_min, err_msg "Request processed: Failure [%d]: %s, err_min, err_msg "Request processed: Offline [%d]: %s, err_min, err_msg
Second patch:
+#define BE_SBUS_REPLY_DATA_INIT { .err_maj = DP_ERR_FATAL, \
.err_min = EFAULT, \.err_msg = "Fatal error" \};ERR_INTERNAL might be a better choice over EFAULT since it basically means we forgot to set it -> code error.
OK, see the attached two.
On 12/04/2015 04:42 PM, 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,7 @@ 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 = name;
I know every call now passes static data but IMHO we should be safe and use talloc_strdup here.
+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");- } else {
DEBUG(SSSDBG_TRACE_LIBS,"Request processed. Returned %d,%d,%s\n",err_maj, err_min, err_msg);- }
Can we translate err_maj into string here? To get message similar to: "Request processed: Success [%d]: %s, err_min, err_msg "Request processed: Failure [%d]: %s, err_min, err_msg "Request processed: Offline [%d]: %s, err_min, err_msg
Second patch:
+#define BE_SBUS_REPLY_DATA_INIT { .err_maj = DP_ERR_FATAL, \
.err_min = EFAULT, \.err_msg = "Fatal error" \};ERR_INTERNAL might be a better choice over EFAULT since it basically means we forgot to set it -> code error.
OK, see the attached two.
Ack.
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,7 @@ 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 = name;
I know every call now passes static data but IMHO we should be safe and use talloc_strdup here.
+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");- } else {
DEBUG(SSSDBG_TRACE_LIBS,"Request processed. Returned %d,%d,%s\n",err_maj, err_min, err_msg);- }
Can we translate err_maj into string here? To get message similar to: "Request processed: Success [%d]: %s, err_min, err_msg "Request processed: Failure [%d]: %s, err_min, err_msg "Request processed: Offline [%d]: %s, err_min, err_msg
Second patch:
+#define BE_SBUS_REPLY_DATA_INIT { .err_maj = DP_ERR_FATAL, \
.err_min = EFAULT, \.err_msg = "Fatal error" \};ERR_INTERNAL might be a better choice over EFAULT since it basically means we forgot to set it -> code error.
OK, see the attached two.
From 0bff8916f35a39ff8fdd4789a31289f7d1b05877 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@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 | 355 +++++++++++++------------------------ src/providers/dp_backend.h | 11 +- src/providers/ipa/ipa_subdomains.c | 2 +- 4 files changed, 138 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..b67efa87008d8e15fb9fb27c3d02996608b0aa62 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,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.
+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");- } 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);- }
- ret = sbus_request_return_and_finish(sbus_req,
DBUS_TYPE_UINT16, &err_maj,DBUS_TYPE_UINT32, &err_min,DBUS_TYPE_STRING, &safe_err_msg,DBUS_TYPE_INVALID);- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,"sbus_request_return_and_finish failed: [%d]: %s\n",ret, sss_strerror(ret));- }
- return ret;
+}
+static errno_t be_sbus_req_reply(struct sbus_request *sbus_req,
int dp_err_type,int errnum,const char *errstr)+{
- dbus_uint16_t err_maj;
- dbus_uint32_t err_min;
- err_maj = dp_err_type;
- err_min = errnum;
- return be_sbus_reply(sbus_req, err_maj, err_min, errstr);
+}
+static void be_req_default_callback(struct be_req *be_req,
int dp_err_type,int errnum,const char *errstr)+{
- struct sbus_request *dbus_req;
- DEBUG(SSSDBG_TRACE_FUNC, "Replying to %s request\n", be_req->req_name);
- dbus_req = (struct sbus_request *) be_req->pvt;
- be_sbus_req_reply(dbus_req, dp_err_type, errnum, errstr);
- talloc_free(be_req);
+}
+/* Send back an immediate reply and set the sbus_request to NULL
- so that we are sure the request is not reused in the future
- */
+static errno_t be_offline_reply(struct sbus_request **sbus_req_ptr) +{
- struct sbus_request *dbus_req;
- errno_t ret;
- if (sbus_req_ptr == NULL) {
return EINVAL;- }
- dbus_req = *sbus_req_ptr;
- ret = be_sbus_req_reply(dbus_req, DP_ERR_OFFLINE, EAGAIN,
"Fast reply - offline");- *sbus_req_ptr = NULL;
- return ret;
+}
void be_terminate_domain_requests(struct be_ctx *be_ctx, const char *domain) { @@ -439,9 +539,6 @@ static void be_queue_next_request(struct be_req *be_req, enum bet_type type) struct bet_queue_item **req_queue; struct sbus_request *dbus_req; int ret;
uint16_t err_maj;
uint32_t err_min;
const char *err_msg = "Cannot file back end request"; struct be_req *next_be_req = NULL;
req_queue = &be_req->becli->bectx->bet_info[type].req_queue;
@@ -481,21 +578,8 @@ static void be_queue_next_request(struct be_req *be_req, enum bet_type type)
dbus_req = (struct sbus_request *) next_be_req->pvt;
- if (dbus_req) {
/* Return a reply if one was requested* There may not be one if this request began* while we were offline*/err_maj = DP_ERR_FATAL;err_min = ret;sbus_request_return_and_finish(dbus_req,DBUS_TYPE_UINT16, &err_maj,DBUS_TYPE_UINT32, &err_min,DBUS_TYPE_STRING, &err_msg,DBUS_TYPE_INVALID);- }
- be_sbus_req_reply(dbus_req, DP_ERR_FATAL, ret,
"Cannot file back end request");}
@@ -663,34 +747,12 @@ static void get_subdomains_callback(struct be_req *req, const char *errstr) { struct sbus_request *dbus_req;
dbus_uint16_t err_maj = 0;
dbus_uint32_t err_min = 0;
const char *err_msg = NULL;
DEBUG(SSSDBG_TRACE_FUNC, "Backend returned: (%d, %d, %s) [%s]\n",
dp_err_type, errnum, errstr?errstr:"<NULL>",dp_err_to_string(dp_err_type));be_queue_next_request(req, BET_SUBDOMAINS);
dbus_req = (struct sbus_request *)req->pvt;
if (dbus_req) {
/* Return a reply if one was requested* There may not be one if this request began* while we were offline*/err_maj = dp_err_type;err_min = errnum;err_msg = safe_be_req_err_msg(errstr, dp_err_type);sbus_request_return_and_finish(dbus_req,DBUS_TYPE_UINT16, &err_maj,DBUS_TYPE_UINT32, &err_min,DBUS_TYPE_STRING, &err_msg,DBUS_TYPE_INVALID);}
dbus_req = (struct sbus_request *) req->pvt;
be_sbus_req_reply(dbus_req, dp_err_type, errnum, errstr); talloc_free(req);
}
@@ -737,7 +799,7 @@ static int be_get_subdomains(struct sbus_request *dbus_req, void *user_data)
/* process request */
- be_req = be_req_create(becli, becli, becli->bectx,
- be_req = be_req_create(becli, becli, becli->bectx, "get subdomains", get_subdomains_callback, dbus_req); if (!be_req) { err_maj = DP_ERR_FATAL;
@@ -797,42 +859,6 @@ immediate: return EOK; }
-static void acctinfo_callback(struct be_req *req,
int dp_err_type,int errnum,const char *errstr)-{
- struct sbus_request *dbus_req;
- dbus_uint16_t err_maj = 0;
- dbus_uint32_t err_min = 0;
- const char *err_msg = NULL;
- dbus_req = (struct sbus_request *)req->pvt;
- if (dbus_req) {
/* Return a reply if one was requested* There may not be one if this request began* while we were offline*/err_maj = dp_err_type;err_min = errnum;err_msg = safe_be_req_err_msg(errstr, dp_err_type);sbus_request_return_and_finish(dbus_req,DBUS_TYPE_UINT16, &err_maj,DBUS_TYPE_UINT32, &err_min,DBUS_TYPE_STRING, &err_msg,DBUS_TYPE_INVALID);DEBUG(SSSDBG_CONF_SETTINGS, "Request processed. Returned %d,%d,%s\n",err_maj, err_min, err_msg);- }
- /* finally free the request */
- talloc_free(req);
-}
struct be_initgr_prereq { char *user; char *domain; @@ -851,8 +877,8 @@ static void acctinfo_callback_initgr_wrap(struct be_req *be_req) struct be_initgr_prereq);
be_req->pvt = pr->orig_pvt_data;
- acctinfo_callback(be_req, pr->orig_dp_err_type,
pr->orig_errnum, pr->orig_errstr);
- be_req_default_callback(be_req, pr->orig_dp_err_type,
pr->orig_errnum, pr->orig_errstr);}
static void acctinfo_callback_initgr_sbus(DBusPendingCall *pending, void *ptr) @@ -1072,7 +1098,7 @@ be_get_account_info_send(TALLOC_CTX *mem_ctx, struct be_get_account_info_state); if (!req) return NULL;
- be_req = be_req_create(state, becli, be_ctx,
- be_req = be_req_create(state, becli, be_ctx, "get account info", be_get_account_info_done, req); if (!be_req) { ret = ENOMEM;
@@ -1187,24 +1213,11 @@ static int be_get_account_info(struct sbus_request *dbus_req, void *user_data) * return offline immediately */ if ((type & BE_REQ_FAST) && becli->bectx->offstat.offline) {
/* Send back an immediate reply */err_maj = DP_ERR_OFFLINE;err_min = EAGAIN;err_msg = "Fast reply - offline";ret = sbus_request_return_and_finish(dbus_req,DBUS_TYPE_UINT16, &err_maj,DBUS_TYPE_UINT32, &err_min,DBUS_TYPE_STRING, &err_msg,DBUS_TYPE_INVALID);
ret = be_offline_reply(&dbus_req); if (ret != EOK) { return ret; }
DEBUG(SSSDBG_CONF_SETTINGS, "Request processed. Returned %d,%d,%s\n",err_maj, err_min, err_msg);dbus_req = NULL; /* This reply will be queued and sent * when we reenter the mainloop. *@@ -1213,8 +1226,8 @@ static int be_get_account_info(struct sbus_request *dbus_req, void *user_data) */ }
- be_req = be_req_create(becli, becli, becli->bectx,
acctinfo_callback, dbus_req);
- be_req = be_req_create(becli, becli, becli->bectx, "get account info",
be_req_default_callback, dbus_req);@@ -1424,7 +1437,7 @@ static int be_pam_handler(struct sbus_request *dbus_req, void *user_data) becli = talloc_get_type(user_data, struct be_client); if (!becli) return EINVAL;
- be_req = be_req_create(becli, becli, becli->bectx,
- be_req = be_req_create(becli, becli, becli->bectx, "PAM", be_pam_handler_callback, dbus_req); if (!be_req) { DEBUG(SSSDBG_TRACE_LIBS, "talloc_zero failed.\n");
@@ -1536,44 +1549,6 @@ done: return EOK; }
-static void be_sudo_handler_reply(struct sbus_request *dbus_req,
dbus_uint16_t dp_err,dbus_uint32_t dp_ret,const char *errstr)-{
- const char *err_msg = NULL;
- if (dbus_req == NULL) {
return;- }
- err_msg = errstr ? errstr : "No errmsg set\n";
- sbus_request_return_and_finish(dbus_req,
DBUS_TYPE_UINT16, &dp_err,DBUS_TYPE_UINT32, &dp_ret,DBUS_TYPE_STRING, &err_msg,DBUS_TYPE_INVALID);- DEBUG(SSSDBG_FUNC_DATA, "SUDO Backend returned: (%d, %d, %s)\n",
dp_err, dp_ret, errstr ? errstr : "<NULL>");-}
-static void be_sudo_handler_callback(struct be_req *req,
int dp_err,int dp_ret,const char *errstr)-{
- const char *err_msg = NULL;
- struct sbus_request *dbus_req;
- dbus_req = (struct sbus_request *)(req->pvt);
- err_msg = safe_be_req_err_msg(errstr, dp_err);
- be_sudo_handler_reply(dbus_req, dp_err, dp_ret, err_msg);
- talloc_free(req);
-}
static int be_sudo_handler(struct sbus_request *dbus_req, void *user_data) { DBusError dbus_error; @@ -1597,8 +1572,8 @@ static int be_sudo_handler(struct sbus_request *dbus_req, void *user_data) }
/* create be request */
- be_req = be_req_create(be_cli, be_cli, be_cli->bectx,
be_sudo_handler_callback, dbus_req);
- be_req = be_req_create(be_cli, be_cli, be_cli->bectx, "sudo",
be_req_default_callback, dbus_req);@@ -1621,9 +1596,8 @@ static int be_sudo_handler(struct sbus_request *dbus_req, void *user_data) * return offline immediately */ if ((type & BE_REQ_FAST) && be_cli->bectx->offstat.offline) {
be_sudo_handler_reply(dbus_req, DP_ERR_OFFLINE, EAGAIN,"Fast reply - offline");be_req->pvt = dbus_req = NULL;
be_offline_reply(&dbus_req);be_req->pvt = NULL; /* This reply will be queued and sent * when we reenter the mainloop. *@@ -1732,16 +1706,11 @@ static int be_sudo_handler(struct sbus_request *dbus_req, void *user_data)
fail: /* send reply back immediately */
- be_sudo_handler_callback(be_req, DP_ERR_FATAL, ret,
err_msg ? err_msg : strerror(ret));
- be_req_default_callback(be_req, DP_ERR_FATAL, ret,
err_msg ? err_msg : strerror(ret));}
-static void be_autofs_handler_callback(struct be_req *req,
int dp_err_type,int errnum,const char *errstr);static int be_autofs_handler(struct sbus_request *dbus_req, void *user_data) { struct be_client *be_cli = NULL; @@ -1773,24 +1742,7 @@ static int be_autofs_handler(struct sbus_request *dbus_req, void *user_data) * return offline immediately */ if ((type & BE_REQ_FAST) && be_cli->bectx->offstat.offline) {
/* Send back an immediate reply */err_maj = DP_ERR_OFFLINE;err_min = EAGAIN;err_msg = "Fast reply - offline";ret = sbus_request_return_and_finish(dbus_req,DBUS_TYPE_UINT16, &err_maj,DBUS_TYPE_UINT32, &err_min,DBUS_TYPE_STRING, &err_msg,DBUS_TYPE_INVALID);if (ret != EOK) {return ret;}DEBUG(SSSDBG_TRACE_LIBS, "Request processed. Returned %d,%d,%s\n",err_maj, err_min, err_msg);dbus_req = NULL;
be_offline_reply(&dbus_req); /* This reply will be queued and sent * when we reenter the mainloop. *@@ -1816,8 +1768,8 @@ static int be_autofs_handler(struct sbus_request *dbus_req, void *user_data) }
/* create be request */
- be_req = be_req_create(be_cli, be_cli, be_cli->bectx,
be_autofs_handler_callback, dbus_req);
- be_req = be_req_create(be_cli, be_cli, be_cli->bectx, "autofs",
be_req_default_callback, dbus_req);@@ -1890,43 +1842,6 @@ done: return EOK; }
-static void be_autofs_handler_callback(struct be_req *req,
int dp_err_type,int errnum,const char *errstr)-{
- struct sbus_request *dbus_req;
- dbus_uint16_t err_maj = 0;
- dbus_uint32_t err_min = 0;
- const char *err_msg = NULL;
- dbus_req = (struct sbus_request *)req->pvt;
- if (dbus_req) {
/* Return a reply if one was requested* There may not be one if this request began* while we were offline*/err_maj = dp_err_type;err_min = errnum;err_msg = safe_be_req_err_msg(errstr, dp_err_type);sbus_request_return_and_finish(dbus_req,DBUS_TYPE_UINT16, &err_maj,DBUS_TYPE_UINT32, &err_min,DBUS_TYPE_STRING, &err_msg,DBUS_TYPE_INVALID);DEBUG(SSSDBG_TRACE_LIBS,"Request processed. Returned %d,%d,%s\n",err_maj, err_min, err_msg);- }
- /* finally free the request */
- talloc_free(req);
-}
static int be_host_handler(struct sbus_request *dbus_req, void *user_data) { struct be_host_req *req; @@ -1958,24 +1873,8 @@ static int be_host_handler(struct sbus_request *dbus_req, void *user_data) */ if ((flags & BE_REQ_FAST) && becli->bectx->offstat.offline) { /* Send back an immediate reply */
err_maj = DP_ERR_OFFLINE;err_min = EAGAIN;err_msg = "Fast reply - offline";
be_offline_reply(&dbus_req);
ret = sbus_request_return_and_finish(dbus_req,DBUS_TYPE_UINT16, &err_maj,DBUS_TYPE_UINT32, &err_min,DBUS_TYPE_STRING, &err_msg,DBUS_TYPE_INVALID);if (ret != EOK) {return ret;}DEBUG(SSSDBG_TRACE_LIBS,"Request processed. Returned %d,%d,%s\n",err_maj, err_min, err_msg);dbus_req = NULL; /* This reply will be queued and sent * when we reenter the mainloop. *@@ -1984,8 +1883,8 @@ static int be_host_handler(struct sbus_request *dbus_req, void *user_data) */ }
- be_req = be_req_create(becli, becli, becli->bectx,
acctinfo_callback, dbus_req);
- be_req = be_req_create(becli, becli, becli->bectx, "hostinfo",
be_req_default_callback, dbus_req);@@ -2257,7 +2156,7 @@ static void check_if_online(struct be_ctx *ctx) goto failed; }
- be_req = be_req_create(ctx, NULL, ctx,
- be_req = be_req_create(ctx, NULL, ctx, "online check", check_online_callback, NULL); if (be_req == NULL) { DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero failed.\n");
diff --git a/src/providers/dp_backend.h b/src/providers/dp_backend.h index f90d0b9c5fe69b1b14caa090bb515c60746de154..cfd8b8aa6d296e87685ce0d503af26f83352fdd0 100644 --- a/src/providers/dp_backend.h +++ b/src/providers/dp_backend.h @@ -291,9 +291,16 @@ errno_t be_res_init(struct be_ctx *ctx);
/* be_req helpers */
+/* Create a back end request and call fn when done. Please note the
- request name is not duplicated. The caller should either provide
- a static string or steal a dynamic string onto req context.
- */
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_ctx *be_req_get_be_ctx(struct be_req *be_req);
void *be_req_get_data(struct be_req *be_req); diff --git a/src/providers/ipa/ipa_subdomains.c b/src/providers/ipa/ipa_subdomains.c index 70a2933757688d0cc758a56d20649bf5e7f43436..b849a7b3d0237e53af62d7131f1bf396f64834ca 100644 --- a/src/providers/ipa/ipa_subdomains.c +++ b/src/providers/ipa/ipa_subdomains.c @@ -1342,7 +1342,7 @@ static void ipa_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, "IPA subdomains", ipa_subdom_be_req_callback, NULL); if (be_req == NULL) { DEBUG(SSSDBG_CRIT_FAILURE, "be_req_create() failed.\n");
-- 2.4.3
From d7b00e1ef4984bb17d4ce62b2c206efebb6aea37 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@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 | 270 +++++++++++++++------------------------ 1 file changed, 102 insertions(+), 168 deletions(-)
diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c index b67efa87008d8e15fb9fb27c3d02996608b0aa62..fa9e86ca6e7d02d6f9f348c0a2054ee69f962f45 100644 --- a/src/providers/data_provider_be.c +++ b/src/providers/data_provider_be.c @@ -342,6 +342,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) { @@ -762,9 +796,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,10 +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"); goto immediate;@@ -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.
err_maj = DP_ERR_OFFLINE;err_min = EAGAIN;err_msg = "Provider is offline";
be_sbus_reply_data_set(&req_reply, DP_ERR_OFFLINE, EAGAIN,"Provider is offline"); goto immediate;
LS
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@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..
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@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..
Thank you http://sssd-ci.duckdns.org/logs/job/34/37/summary.html
master: * 0741237b3f9209af43d956216b3c2f09b90c4ebc * 4afc1f2b6ca066d30d2be5ccda9fa760b5a6016e
LS
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@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@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@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
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@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@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@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, ENODEVDo 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); }
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@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@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@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, ENODEVDo 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",
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@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@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@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, ENODEVDo 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");
I'm fine with such change.
Any other objections?
LS
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@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@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@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, ENODEVDo 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");I'm fine with such change.
Any other objections?
I'll send that patch so we can review it properly with CI and stuff..
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@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@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@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, ENODEVDo 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
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina -
Petr Cech