On (10/08/15 19:28), Michal Židek wrote:
See attached patch that adds debug messages
Michal
From 8d3a54ee023ccb5bc37201a7c2fb1ec671134c1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Mon, 10 Aug 2015 18:35:16 +0200 Subject: [PATCH] DEBUG: Add new debug category for fail over.
src/providers/data_provider_fo.c | 8 ++++---- src/providers/dp_backend.h | 27 +++++++++++++++++++++++---- src/util/util.h | 1 + 3 files changed, 28 insertions(+), 8 deletions(-)
diff --git a/src/providers/data_provider_fo.c b/src/providers/data_provider_fo.c index dab796d..de8a223 100644 --- a/src/providers/data_provider_fo.c +++ b/src/providers/data_provider_fo.c @@ -743,10 +743,10 @@ void reset_fo(struct be_ctx *be_ctx) fo_reset_services(be_ctx->be_fo->fo_ctx); }
-void be_fo_set_port_status(struct be_ctx *ctx,
const char *service_name,struct fo_server *server,enum port_status status)+void _be_fo_set_port_status(struct be_ctx *ctx,
const char *service_name,struct fo_server *server,enum port_status status){ struct be_svc_data *be_svc;
diff --git a/src/providers/dp_backend.h b/src/providers/dp_backend.h index e4e22ea..18a78a0 100644 --- a/src/providers/dp_backend.h +++ b/src/providers/dp_backend.h @@ -259,10 +259,29 @@ struct tevent_req *be_resolve_server_send(TALLOC_CTX *memctx, bool first_try); int be_resolve_server_recv(struct tevent_req *req, struct fo_server **srv);
-void be_fo_set_port_status(struct be_ctx *ctx,
const char *service_name,struct fo_server *server,enum port_status status);+#define be_fo_set_port_status(ctx, \
You can change be_fo_set_port_status to new static function with prefix "_" and backwark compatible function wit the same name will be thin wrapper around it. We should not abuse macro.
If you are woried about performance it's better to reply on gcc and inlining of functions. It's better to rely on type check of functions.
service_name, \server, \status) \+do { \
- switch (status) { \
- case PORT_NEUTRAL: \
DEBUG(SSSDBG_BE_FO, "Setting status: PORT_NEUTRAL\n"); \break; \- case PORT_WORKING: \
DEBUG(SSSDBG_BE_FO, "Setting status: PORT_WORKING\n"); \break; \- case PORT_NOT_WORKING: \
DEBUG(SSSDBG_BE_FO, "Setting status: PORT_NOT_WORKING\n"); \break; \- } \
I still
- _be_fo_set_port_status(ctx, service_name, server, status); \
+} while (0)
+void _be_fo_set_port_status(struct be_ctx *ctx,
const char *service_name,struct fo_server *server,enum port_status status);/*
- Instruct fail-over to try next server on the next connect attempt.
diff --git a/src/util/util.h b/src/util/util.h index 94a3dde..aac91c8 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -92,6 +92,7 @@ int get_fd_from_debug_file(void); #define SSSDBG_TRACE_LIBS 0x1000 /* level 7 */ #define SSSDBG_TRACE_INTERNAL 0x2000 /* level 8 */ #define SSSDBG_TRACE_ALL 0x4000 /* level 9 */ +#define SSSDBG_BE_FO 0x8000
Could you also map new debug level to old one? (probably 9) So pleaple who are used to for old style debug_levels will not miss these debug messages.
LS
On 08/11/2015 06:52 AM, Lukas Slebodnik wrote:
On (10/08/15 19:28), Michal Židek wrote:
See attached patch that adds debug messages
Michal
Sorry that version of the patch was wrong.
The attached is a proper version.
From 8d3a54ee023ccb5bc37201a7c2fb1ec671134c1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Mon, 10 Aug 2015 18:35:16 +0200 Subject: [PATCH] DEBUG: Add new debug category for fail over.
src/providers/data_provider_fo.c | 8 ++++---- src/providers/dp_backend.h | 27 +++++++++++++++++++++++---- src/util/util.h | 1 + 3 files changed, 28 insertions(+), 8 deletions(-)
diff --git a/src/providers/data_provider_fo.c b/src/providers/data_provider_fo.c index dab796d..de8a223 100644 --- a/src/providers/data_provider_fo.c +++ b/src/providers/data_provider_fo.c @@ -743,10 +743,10 @@ void reset_fo(struct be_ctx *be_ctx) fo_reset_services(be_ctx->be_fo->fo_ctx); }
-void be_fo_set_port_status(struct be_ctx *ctx,
const char *service_name,struct fo_server *server,enum port_status status)+void _be_fo_set_port_status(struct be_ctx *ctx,
const char *service_name,struct fo_server *server,enum port_status status){ struct be_svc_data *be_svc;
diff --git a/src/providers/dp_backend.h b/src/providers/dp_backend.h index e4e22ea..18a78a0 100644 --- a/src/providers/dp_backend.h +++ b/src/providers/dp_backend.h @@ -259,10 +259,29 @@ struct tevent_req *be_resolve_server_send(TALLOC_CTX *memctx, bool first_try); int be_resolve_server_recv(struct tevent_req *req, struct fo_server **srv);
-void be_fo_set_port_status(struct be_ctx *ctx,
const char *service_name,struct fo_server *server,enum port_status status);+#define be_fo_set_port_status(ctx, \
You can change be_fo_set_port_status to new static function with prefix "_" and backwark compatible function wit the same name will be thin wrapper around it. We should not abuse macro.
If you are woried about performance it's better to reply on gcc and inlining of functions. It's better to rely on type check of functions.
No. The puprpose of the patch (what you could not see from the old version) was to print the line and file from which the function was called. This can only be done using the macro.
service_name, \server, \status) \+do { \
- switch (status) { \
- case PORT_NEUTRAL: \
DEBUG(SSSDBG_BE_FO, "Setting status: PORT_NEUTRAL\n"); \break; \- case PORT_WORKING: \
DEBUG(SSSDBG_BE_FO, "Setting status: PORT_WORKING\n"); \break; \- case PORT_NOT_WORKING: \
DEBUG(SSSDBG_BE_FO, "Setting status: PORT_NOT_WORKING\n"); \break; \- } \
I still
You probably forgot to finish your sentence here.
- _be_fo_set_port_status(ctx, service_name, server, status); \
+} while (0)
+void _be_fo_set_port_status(struct be_ctx *ctx,
const char *service_name,struct fo_server *server,enum port_status status);/*
- Instruct fail-over to try next server on the next connect attempt.
diff --git a/src/util/util.h b/src/util/util.h index 94a3dde..aac91c8 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -92,6 +92,7 @@ int get_fd_from_debug_file(void); #define SSSDBG_TRACE_LIBS 0x1000 /* level 7 */ #define SSSDBG_TRACE_INTERNAL 0x2000 /* level 8 */ #define SSSDBG_TRACE_ALL 0x4000 /* level 9 */ +#define SSSDBG_BE_FO 0x8000
Could you also map new debug level to old one? (probably 9) So pleaple who are used to for old style debug_levels will not miss these debug messages.
I was not sure if I should, but since level 9 is already noisy level it will not much harm if I add the mapping to 9.
LS
Thanks, Michal
On (11/08/15 13:16), Michal Židek wrote:
On 08/11/2015 06:52 AM, Lukas Slebodnik wrote:
On (10/08/15 19:28), Michal Židek wrote:
See attached patch that adds debug messages
Michal
Sorry that version of the patch was wrong.
The attached is a proper version.
From 8d3a54ee023ccb5bc37201a7c2fb1ec671134c1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Mon, 10 Aug 2015 18:35:16 +0200 Subject: [PATCH] DEBUG: Add new debug category for fail over.
src/providers/data_provider_fo.c | 8 ++++---- src/providers/dp_backend.h | 27 +++++++++++++++++++++++---- src/util/util.h | 1 + 3 files changed, 28 insertions(+), 8 deletions(-)
diff --git a/src/providers/data_provider_fo.c b/src/providers/data_provider_fo.c index dab796d..de8a223 100644 --- a/src/providers/data_provider_fo.c +++ b/src/providers/data_provider_fo.c @@ -743,10 +743,10 @@ void reset_fo(struct be_ctx *be_ctx) fo_reset_services(be_ctx->be_fo->fo_ctx); }
-void be_fo_set_port_status(struct be_ctx *ctx,
const char *service_name,struct fo_server *server,enum port_status status)+void _be_fo_set_port_status(struct be_ctx *ctx,
const char *service_name,struct fo_server *server,enum port_status status){ struct be_svc_data *be_svc;
diff --git a/src/providers/dp_backend.h b/src/providers/dp_backend.h index e4e22ea..18a78a0 100644 --- a/src/providers/dp_backend.h +++ b/src/providers/dp_backend.h @@ -259,10 +259,29 @@ struct tevent_req *be_resolve_server_send(TALLOC_CTX *memctx, bool first_try); int be_resolve_server_recv(struct tevent_req *req, struct fo_server **srv);
-void be_fo_set_port_status(struct be_ctx *ctx,
const char *service_name,struct fo_server *server,enum port_status status);+#define be_fo_set_port_status(ctx, \
You can change be_fo_set_port_status to new static function with prefix "_" and backwark compatible function wit the same name will be thin wrapper around it. We should not abuse macro.
If you are woried about performance it's better to reply on gcc and inlining of functions. It's better to rely on type check of functions.
No. The puprpose of the patch (what you could not see from the old version) was to print the line and file from which the function was called. This can only be done using the macro.
It can be achieved even without complicated do while macro.
#define be_fo_set_port_status(ctx, service_name, server, status) \ _be_fo_set_port_status(ctx, service_name, server, status, \ __FILE__, __LINE__)
and log messages will be added to _be_fo_set_port_status.
The macro will much simpler and any compilation errors/typos can be easily found in the function _be_fo_set_port_status.
@more inspiration -> talloc.h or tevent.h
LS
On 08/11/2015 01:25 PM, Lukas Slebodnik wrote:
On (11/08/15 13:16), Michal Židek wrote:
On 08/11/2015 06:52 AM, Lukas Slebodnik wrote:
On (10/08/15 19:28), Michal Židek wrote:
See attached patch that adds debug messages
Michal
Sorry that version of the patch was wrong.
The attached is a proper version.
From 8d3a54ee023ccb5bc37201a7c2fb1ec671134c1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Mon, 10 Aug 2015 18:35:16 +0200 Subject: [PATCH] DEBUG: Add new debug category for fail over.
src/providers/data_provider_fo.c | 8 ++++---- src/providers/dp_backend.h | 27 +++++++++++++++++++++++---- src/util/util.h | 1 + 3 files changed, 28 insertions(+), 8 deletions(-)
diff --git a/src/providers/data_provider_fo.c b/src/providers/data_provider_fo.c index dab796d..de8a223 100644 --- a/src/providers/data_provider_fo.c +++ b/src/providers/data_provider_fo.c @@ -743,10 +743,10 @@ void reset_fo(struct be_ctx *be_ctx) fo_reset_services(be_ctx->be_fo->fo_ctx); }
-void be_fo_set_port_status(struct be_ctx *ctx,
const char *service_name,struct fo_server *server,enum port_status status)+void _be_fo_set_port_status(struct be_ctx *ctx,
const char *service_name,struct fo_server *server,enum port_status status){ struct be_svc_data *be_svc;
diff --git a/src/providers/dp_backend.h b/src/providers/dp_backend.h index e4e22ea..18a78a0 100644 --- a/src/providers/dp_backend.h +++ b/src/providers/dp_backend.h @@ -259,10 +259,29 @@ struct tevent_req *be_resolve_server_send(TALLOC_CTX *memctx, bool first_try); int be_resolve_server_recv(struct tevent_req *req, struct fo_server **srv);
-void be_fo_set_port_status(struct be_ctx *ctx,
const char *service_name,struct fo_server *server,enum port_status status);+#define be_fo_set_port_status(ctx, \
You can change be_fo_set_port_status to new static function with prefix "_" and backwark compatible function wit the same name will be thin wrapper around it. We should not abuse macro.
If you are woried about performance it's better to reply on gcc and inlining of functions. It's better to rely on type check of functions.
No. The puprpose of the patch (what you could not see from the old version) was to print the line and file from which the function was called. This can only be done using the macro.
It can be achieved even without complicated do while macro.
#define be_fo_set_port_status(ctx, service_name, server, status) \ _be_fo_set_port_status(ctx, service_name, server, status, \ __FILE__, __LINE__)
and log messages will be added to _be_fo_set_port_status.
The macro will much simpler and any compilation errors/typos can be easily found in the function _be_fo_set_port_status.
@more inspiration -> talloc.h or tevent.h
LS
I agree that the code would look better, but would it not make the debug message a little too long with unnecessary information? We would have to pass also the __FUNCTION__ and another function name (be_fo_set_port_status) would be printed by default using the DEBUG). Would that be OK?
Michal
On (11/08/15 15:00), Michal Židek wrote:
On 08/11/2015 01:25 PM, Lukas Slebodnik wrote:
On (11/08/15 13:16), Michal Židek wrote:
On 08/11/2015 06:52 AM, Lukas Slebodnik wrote:
On (10/08/15 19:28), Michal Židek wrote:
See attached patch that adds debug messages
Michal
Sorry that version of the patch was wrong.
The attached is a proper version.
From 8d3a54ee023ccb5bc37201a7c2fb1ec671134c1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Mon, 10 Aug 2015 18:35:16 +0200 Subject: [PATCH] DEBUG: Add new debug category for fail over.
src/providers/data_provider_fo.c | 8 ++++---- src/providers/dp_backend.h | 27 +++++++++++++++++++++++---- src/util/util.h | 1 + 3 files changed, 28 insertions(+), 8 deletions(-)
diff --git a/src/providers/data_provider_fo.c b/src/providers/data_provider_fo.c index dab796d..de8a223 100644 --- a/src/providers/data_provider_fo.c +++ b/src/providers/data_provider_fo.c @@ -743,10 +743,10 @@ void reset_fo(struct be_ctx *be_ctx) fo_reset_services(be_ctx->be_fo->fo_ctx); }
-void be_fo_set_port_status(struct be_ctx *ctx,
const char *service_name,struct fo_server *server,enum port_status status)+void _be_fo_set_port_status(struct be_ctx *ctx,
const char *service_name,struct fo_server *server,enum port_status status){ struct be_svc_data *be_svc;
diff --git a/src/providers/dp_backend.h b/src/providers/dp_backend.h index e4e22ea..18a78a0 100644 --- a/src/providers/dp_backend.h +++ b/src/providers/dp_backend.h @@ -259,10 +259,29 @@ struct tevent_req *be_resolve_server_send(TALLOC_CTX *memctx, bool first_try); int be_resolve_server_recv(struct tevent_req *req, struct fo_server **srv);
-void be_fo_set_port_status(struct be_ctx *ctx,
const char *service_name,struct fo_server *server,enum port_status status);+#define be_fo_set_port_status(ctx, \
You can change be_fo_set_port_status to new static function with prefix "_" and backwark compatible function wit the same name will be thin wrapper around it. We should not abuse macro.
If you are woried about performance it's better to reply on gcc and inlining of functions. It's better to rely on type check of functions.
No. The puprpose of the patch (what you could not see from the old version) was to print the line and file from which the function was called. This can only be done using the macro.
It can be achieved even without complicated do while macro.
#define be_fo_set_port_status(ctx, service_name, server, status) \ _be_fo_set_port_status(ctx, service_name, server, status, \ __FILE__, __LINE__)
and log messages will be added to _be_fo_set_port_status.
The macro will much simpler and any compilation errors/typos can be easily found in the function _be_fo_set_port_status.
@more inspiration -> talloc.h or tevent.h
LS
I agree that the code would look better, but would it not make the debug message a little too long
We do not have a limit for line length in log file. Some messages are very long; especially if they contains DN or LDAP filter.
LS
On 08/12/2015 06:24 AM, Lukas Slebodnik wrote:
On (11/08/15 15:00), Michal Židek wrote:
On 08/11/2015 01:25 PM, Lukas Slebodnik wrote:
On (11/08/15 13:16), Michal Židek wrote:
On 08/11/2015 06:52 AM, Lukas Slebodnik wrote:
On (10/08/15 19:28), Michal Židek wrote:
See attached patch that adds debug messages
Michal
Sorry that version of the patch was wrong.
The attached is a proper version.
From 8d3a54ee023ccb5bc37201a7c2fb1ec671134c1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Mon, 10 Aug 2015 18:35:16 +0200 Subject: [PATCH] DEBUG: Add new debug category for fail over.
src/providers/data_provider_fo.c | 8 ++++---- src/providers/dp_backend.h | 27 +++++++++++++++++++++++---- src/util/util.h | 1 + 3 files changed, 28 insertions(+), 8 deletions(-)
diff --git a/src/providers/data_provider_fo.c b/src/providers/data_provider_fo.c index dab796d..de8a223 100644 --- a/src/providers/data_provider_fo.c +++ b/src/providers/data_provider_fo.c @@ -743,10 +743,10 @@ void reset_fo(struct be_ctx *be_ctx) fo_reset_services(be_ctx->be_fo->fo_ctx); }
-void be_fo_set_port_status(struct be_ctx *ctx,
const char *service_name,struct fo_server *server,enum port_status status)+void _be_fo_set_port_status(struct be_ctx *ctx,
const char *service_name,struct fo_server *server,enum port_status status){ struct be_svc_data *be_svc;
diff --git a/src/providers/dp_backend.h b/src/providers/dp_backend.h index e4e22ea..18a78a0 100644 --- a/src/providers/dp_backend.h +++ b/src/providers/dp_backend.h @@ -259,10 +259,29 @@ struct tevent_req *be_resolve_server_send(TALLOC_CTX *memctx, bool first_try); int be_resolve_server_recv(struct tevent_req *req, struct fo_server **srv);
-void be_fo_set_port_status(struct be_ctx *ctx,
const char *service_name,struct fo_server *server,enum port_status status);+#define be_fo_set_port_status(ctx, \
You can change be_fo_set_port_status to new static function with prefix "_" and backwark compatible function wit the same name will be thin wrapper around it. We should not abuse macro.
If you are woried about performance it's better to reply on gcc and inlining of functions. It's better to rely on type check of functions.
No. The puprpose of the patch (what you could not see from the old version) was to print the line and file from which the function was called. This can only be done using the macro.
It can be achieved even without complicated do while macro.
#define be_fo_set_port_status(ctx, service_name, server, status) \ _be_fo_set_port_status(ctx, service_name, server, status, \ __FILE__, __LINE__)
and log messages will be added to _be_fo_set_port_status.
The macro will much simpler and any compilation errors/typos can be easily found in the function _be_fo_set_port_status.
@more inspiration -> talloc.h or tevent.h
LS
I agree that the code would look better, but would it not make the debug message a little too long
We do not have a limit for line length in log file. Some messages are very long; especially if they contains DN or LDAP filter.
True. And it indeed looks much better this way.
New patch attached.
Michal
On (12/08/15 19:09), Michal Židek wrote:
On 08/12/2015 06:24 AM, Lukas Slebodnik wrote:
On (11/08/15 15:00), Michal Židek wrote:
On 08/11/2015 01:25 PM, Lukas Slebodnik wrote:
On (11/08/15 13:16), Michal Židek wrote:
On 08/11/2015 06:52 AM, Lukas Slebodnik wrote:
On (10/08/15 19:28), Michal Židek wrote: >See attached patch that adds debug messages > > >Michal
Sorry that version of the patch was wrong.
The attached is a proper version.
>From 8d3a54ee023ccb5bc37201a7c2fb1ec671134c1f Mon Sep 17 00:00:00 2001 >From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com >Date: Mon, 10 Aug 2015 18:35:16 +0200 >Subject: [PATCH] DEBUG: Add new debug category for fail over. > >--- >src/providers/data_provider_fo.c | 8 ++++---- >src/providers/dp_backend.h | 27 +++++++++++++++++++++++---- >src/util/util.h | 1 + >3 files changed, 28 insertions(+), 8 deletions(-) > >diff --git a/src/providers/data_provider_fo.c b/src/providers/data_provider_fo.c >index dab796d..de8a223 100644 >--- a/src/providers/data_provider_fo.c >+++ b/src/providers/data_provider_fo.c >@@ -743,10 +743,10 @@ void reset_fo(struct be_ctx *be_ctx) > fo_reset_services(be_ctx->be_fo->fo_ctx); >} > >-void be_fo_set_port_status(struct be_ctx *ctx, >- const char *service_name, >- struct fo_server *server, >- enum port_status status) >+void _be_fo_set_port_status(struct be_ctx *ctx, >+ const char *service_name, >+ struct fo_server *server, >+ enum port_status status) >{ > struct be_svc_data *be_svc; > >diff --git a/src/providers/dp_backend.h b/src/providers/dp_backend.h >index e4e22ea..18a78a0 100644 >--- a/src/providers/dp_backend.h >+++ b/src/providers/dp_backend.h >@@ -259,10 +259,29 @@ struct tevent_req *be_resolve_server_send(TALLOC_CTX *memctx, > bool first_try); >int be_resolve_server_recv(struct tevent_req *req, struct fo_server **srv); > >-void be_fo_set_port_status(struct be_ctx *ctx, >- const char *service_name, >- struct fo_server *server, >- enum port_status status); >+#define be_fo_set_port_status(ctx, \ You can change be_fo_set_port_status to new static function with prefix "_" and backwark compatible function wit the same name will be thin wrapper around it. We should not abuse macro.
If you are woried about performance it's better to reply on gcc and inlining of functions. It's better to rely on type check of functions.
No. The puprpose of the patch (what you could not see from the old version) was to print the line and file from which the function was called. This can only be done using the macro.
It can be achieved even without complicated do while macro.
#define be_fo_set_port_status(ctx, service_name, server, status) \ _be_fo_set_port_status(ctx, service_name, server, status, \ __FILE__, __LINE__)
and log messages will be added to _be_fo_set_port_status.
The macro will much simpler and any compilation errors/typos can be easily found in the function _be_fo_set_port_status.
@more inspiration -> talloc.h or tevent.h
LS
I agree that the code would look better, but would it not make the debug message a little too long
We do not have a limit for line length in log file. Some messages are very long; especially if they contains DN or LDAP filter.
True. And it indeed looks much better this way.
otherwise I would not propose it.
New patch attached.
Michal
From 0aec877fb0d773a98a9d628aa1d9a89062ab0b9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Mon, 10 Aug 2015 18:35:16 +0200 Subject: [PATCH] DEBUG: Add new debug category for fail over.
src/providers/data_provider_fo.c | 30 ++++++++++++++++++++++++++---- src/providers/dp_backend.h | 15 +++++++++++---- src/tests/debug-tests.c | 2 +- src/util/debug.c | 2 +- src/util/util.h | 1 + 5 files changed, 40 insertions(+), 10 deletions(-)
ACK
LS
On 08/12/2015 07:09 PM, Michal Židek wrote:
On 08/12/2015 06:24 AM, Lukas Slebodnik wrote:
On (11/08/15 15:00), Michal Židek wrote:
On 08/11/2015 01:25 PM, Lukas Slebodnik wrote:
On (11/08/15 13:16), Michal Židek wrote:
On 08/11/2015 06:52 AM, Lukas Slebodnik wrote:
On (10/08/15 19:28), Michal Židek wrote: > See attached patch that adds debug messages > > > Michal
Sorry that version of the patch was wrong.
The attached is a proper version.
>From 8d3a54ee023ccb5bc37201a7c2fb1ec671134c1f Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com > Date: Mon, 10 Aug 2015 18:35:16 +0200 > Subject: [PATCH] DEBUG: Add new debug category for fail over. > > --- > src/providers/data_provider_fo.c | 8 ++++---- > src/providers/dp_backend.h | 27 +++++++++++++++++++++++---- > src/util/util.h | 1 + > 3 files changed, 28 insertions(+), 8 deletions(-) > > diff --git a/src/providers/data_provider_fo.c > b/src/providers/data_provider_fo.c > index dab796d..de8a223 100644 > --- a/src/providers/data_provider_fo.c > +++ b/src/providers/data_provider_fo.c > @@ -743,10 +743,10 @@ void reset_fo(struct be_ctx *be_ctx) > fo_reset_services(be_ctx->be_fo->fo_ctx); > } > > -void be_fo_set_port_status(struct be_ctx *ctx, > - const char *service_name, > - struct fo_server *server, > - enum port_status status) > +void _be_fo_set_port_status(struct be_ctx *ctx, > + const char *service_name, > + struct fo_server *server, > + enum port_status status) > { > struct be_svc_data *be_svc; > > diff --git a/src/providers/dp_backend.h b/src/providers/dp_backend.h > index e4e22ea..18a78a0 100644 > --- a/src/providers/dp_backend.h > +++ b/src/providers/dp_backend.h > @@ -259,10 +259,29 @@ struct tevent_req > *be_resolve_server_send(TALLOC_CTX *memctx, > bool first_try); > int be_resolve_server_recv(struct tevent_req *req, struct > fo_server **srv); > > -void be_fo_set_port_status(struct be_ctx *ctx, > - const char *service_name, > - struct fo_server *server, > - enum port_status status); > +#define be_fo_set_port_status(ctx, \ You can change be_fo_set_port_status to new static function with prefix "_" and backwark compatible function wit the same name will be thin wrapper around it. We should not abuse macro.
If you are woried about performance it's better to reply on gcc and inlining of functions. It's better to rely on type check of functions.
No. The puprpose of the patch (what you could not see from the old version) was to print the line and file from which the function was called. This can only be done using the macro.
It can be achieved even without complicated do while macro.
#define be_fo_set_port_status(ctx, service_name, server, status) \ _be_fo_set_port_status(ctx, service_name, server, status, \ __FILE__, __LINE__)
and log messages will be added to _be_fo_set_port_status.
The macro will much simpler and any compilation errors/typos can be easily found in the function _be_fo_set_port_status.
@more inspiration -> talloc.h or tevent.h
LS
I agree that the code would look better, but would it not make the debug message a little too long
We do not have a limit for line length in log file. Some messages are very long; especially if they contains DN or LDAP filter.
True. And it indeed looks much better this way.
New patch attached.
Michal
Kudos for adding new level. Can you also create a patch, that would change the debug level constants to 0x00000000 format so it is clear that it is 32bits and there is still lots of space to add new levels?
On 08/13/2015 03:22 PM, Pavel Březina wrote:
On 08/12/2015 07:09 PM, Michal Židek wrote:
On 08/12/2015 06:24 AM, Lukas Slebodnik wrote:
On (11/08/15 15:00), Michal Židek wrote:
On 08/11/2015 01:25 PM, Lukas Slebodnik wrote:
On (11/08/15 13:16), Michal Židek wrote:
On 08/11/2015 06:52 AM, Lukas Slebodnik wrote: > On (10/08/15 19:28), Michal Židek wrote: >> See attached patch that adds debug messages >> >> >> Michal >
Sorry that version of the patch was wrong.
The attached is a proper version.
> >From 8d3a54ee023ccb5bc37201a7c2fb1ec671134c1f Mon Sep 17 00:00:00 > 2001 >> From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com >> Date: Mon, 10 Aug 2015 18:35:16 +0200 >> Subject: [PATCH] DEBUG: Add new debug category for fail over. >> >> --- >> src/providers/data_provider_fo.c | 8 ++++---- >> src/providers/dp_backend.h | 27 +++++++++++++++++++++++---- >> src/util/util.h | 1 + >> 3 files changed, 28 insertions(+), 8 deletions(-) >> >> diff --git a/src/providers/data_provider_fo.c >> b/src/providers/data_provider_fo.c >> index dab796d..de8a223 100644 >> --- a/src/providers/data_provider_fo.c >> +++ b/src/providers/data_provider_fo.c >> @@ -743,10 +743,10 @@ void reset_fo(struct be_ctx *be_ctx) >> fo_reset_services(be_ctx->be_fo->fo_ctx); >> } >> >> -void be_fo_set_port_status(struct be_ctx *ctx, >> - const char *service_name, >> - struct fo_server *server, >> - enum port_status status) >> +void _be_fo_set_port_status(struct be_ctx *ctx, >> + const char *service_name, >> + struct fo_server *server, >> + enum port_status status) >> { >> struct be_svc_data *be_svc; >> >> diff --git a/src/providers/dp_backend.h >> b/src/providers/dp_backend.h >> index e4e22ea..18a78a0 100644 >> --- a/src/providers/dp_backend.h >> +++ b/src/providers/dp_backend.h >> @@ -259,10 +259,29 @@ struct tevent_req >> *be_resolve_server_send(TALLOC_CTX *memctx, >> bool first_try); >> int be_resolve_server_recv(struct tevent_req *req, struct >> fo_server **srv); >> >> -void be_fo_set_port_status(struct be_ctx *ctx, >> - const char *service_name, >> - struct fo_server *server, >> - enum port_status status); >> +#define be_fo_set_port_status(ctx, \ > You can change be_fo_set_port_status to new static function with > prefix "_" > and backwark compatible function wit the same name will be thin > wrapper > around it. We should not abuse macro. > > If you are woried about performance it's better to reply on gcc > and inlining of > functions. It's better to rely on type check of functions. >
No. The puprpose of the patch (what you could not see from the old version) was to print the line and file from which the function was called. This can only be done using the macro.
It can be achieved even without complicated do while macro.
#define be_fo_set_port_status(ctx, service_name, server, status) \ _be_fo_set_port_status(ctx, service_name, server, status, \ __FILE__, __LINE__)
and log messages will be added to _be_fo_set_port_status.
The macro will much simpler and any compilation errors/typos can be easily found in the function _be_fo_set_port_status.
@more inspiration -> talloc.h or tevent.h
LS
I agree that the code would look better, but would it not make the debug message a little too long
We do not have a limit for line length in log file. Some messages are very long; especially if they contains DN or LDAP filter.
True. And it indeed looks much better this way.
New patch attached.
Michal
Kudos for adding new level. Can you also create a patch, that would change the debug level constants to 0x00000000 format so it is clear that it is 32bits and there is still lots of space to add new levels?
Will do. I was thinking if I should do it with this patch or not. But because we have one more level unused I decided to postpone it. But I will do it.
Michal
On Thu, Aug 13, 2015 at 07:18:27AM +0200, Lukas Slebodnik wrote:
From 0aec877fb0d773a98a9d628aa1d9a89062ab0b9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Mon, 10 Aug 2015 18:35:16 +0200 Subject: [PATCH] DEBUG: Add new debug category for fail over.
src/providers/data_provider_fo.c | 30 ++++++++++++++++++++++++++---- src/providers/dp_backend.h | 15 +++++++++++---- src/tests/debug-tests.c | 2 +- src/util/debug.c | 2 +- src/util/util.h | 1 + 5 files changed, 40 insertions(+), 10 deletions(-)
ACK
LS
* master: c4fb8f55f2894de431478ccfec63f9a97e090d0e
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Michal Židek -
Pavel Březina