[PATCH] DEBUG: Add new debug category for fail over - sssd-devel - Fedora Mailing-Lists

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

[PATCH] DEBUG: Add new debug category for fail over

IFP: use default limit if...

[PATCH] SYSDB: add more debug...

Michal Židek

Monday, 10 August 2015 Mon, 10 Aug '15

12:28 p.m.

See attached patch that adds debug messages Michal

Attachments:

0001-DEBUG-Add-new-debug-category-for-fail-over.patch (text/x-patch — 3.1 KB)

Reply

Show replies by thread

Lukas Slebodnik

Monday, 10 August Mon, 10 Aug

11:52 p.m.

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(a)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

Reply

Michal Židek

Tuesday, 11 August Tue, 11 Aug

6:16 a.m.

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(a)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

Reply

Lukas Slebodnik

6:25 a.m.

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(a)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

Reply

Michal Židek

8 a.m.

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(a)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

Reply

Lukas Slebodnik

11:24 p.m.

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(a)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

Reply

Michal Židek

Wednesday, 12 August Wed, 12 Aug

12:09 p.m.

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(a)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

Reply

Lukas Slebodnik

Thursday, 13 August Thu, 13 Aug

12:18 a.m.

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(a)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(a)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

Reply

Pavel Březina

8:22 a.m.

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(a)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?

Reply

Michal Židek

8:49 a.m.

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(a)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

Reply

Jakub Hrozek

Friday, 14 August Fri, 14 Aug

3:44 p.m.

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(a)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

Reply

3175

days inactive

3179

days old

sssd-devel@lists.fedorahosted.org

Manage subscription

10 comments

4 participants

tags (0)

participants (4)

Jakub Hrozek
Lukas Slebodnik
Michal Židek
Pavel Březina