A set of patches to sanitize logger code a little bit. #5535

alexey-tikhonov · 2021-03-12T19:17:17Z

Most significant changes are:

getting rid of 'debug-to-files', 'debug-to-stderr' command line options and undocumented 'debug_to_files' sssd.conf option
and
getting sss_set_logger() and open_debug_file() incorporated into DEBUG_INIT API

This PR also resolves: #5488

defined in debug.c from util.h to debug.h

This patch gets rid of: - 'debug-to-files', 'debug-to-stderr' command line options - undocumented 'debug_to_files' sssd.conf option and makes '--logger' command line option the only "source of truth" for logger type configuration. Those options were not used much anyway but made precedence logic obscure in case contradictory settings were used. :config: Long time deprecated and undocumented 'debug_to_files' option was removed. :relnote: 'debug-to-files', 'debug-to-stderr' command line and undocumented 'debug_to_files' config options were removed.

This makes code less error-prone reducing amount of function calls required for debug initialization.

and moved rarely used / "private" functions to the bottom.

since is it is only used in tests.

This makes code less error-prone reducing amount of function calls required for debug initialization.

Resolves: SSSD#5488

thalman · 2021-03-24T16:40:59Z

I went trough the patches, looks good to me. I will run few tests before acking.

Thanks
Tom

thalman · 2021-03-31T11:32:20Z

Just for the record: Testing on my fedora shows that the patch as is prevents sssd to log into files in our current setup.
We discussed it on IRC and @alexey-tikhonov will send an update later.

Thanks
Tom

if its value specified explictly

alexey-tikhonov · 2021-03-31T13:02:44Z

Thanks. Updated.

thalman · 2021-03-31T15:08:32Z

Thanks for the patch
ACK

pbrezina · 2021-04-01T09:36:37Z

Pushed PR: #5535

master
- 66960c7 - MONITOR: in case '-i' is given don't force logger to 'stderr' if its value specified explictly
- 0cddb67 - DEBUG: introduce SSSDBG_TOOLS_DEFAULT
- 21334de - MONITOR: added logging of cmd used to start services
- dde57f7 - DEBUG: incorporate open_debug_file() into DEBUG_INIT
- 374d644 - Moved SSSDBG_MASK_ALL out of debug.h since is it is only used in tests.
- cf69917 - DEBUG: added several comments to debug.h API and moved rarely used / "private" functions to the bottom.
- 4d133e1 - DEBUG: remove sss_set_logger() from public API
- c14e439 - DEBUG: incorporate sss_set_logger() into DEBUG_INIT
- fc5b64e - DEBUG: make use of existing SSSD_DEBUG_OPTS macro
- fee3883 - DEBUG: use '--logger' as the only option to configure logger type.
- 0dfb188 - Moved declaration of debug related helpers defined in debug.c from util.h to debug.h
- 0fd0681 - Moved ldb_debug_messages() out of UTILS to SYSDB

joakim-tjernlund · 2021-04-02T09:42:54Z

This PR forgot to change -f in src/sysv/gentoo/sssd.in so sssd now fails to start as -f option is not recognized.
Please replace -f with --logger=files

alexey-tikhonov · 2021-04-06T09:14:43Z

This PR forgot to change -f in src/sysv/gentoo/sssd.in so sssd now fails to start as -f option is not recognized.
Please replace -f with --logger=files

@joakim-tjernlund , do we really need distribution-specific config in upstream repo?

@scabrero , do you use https://github.com/SSSD/sssd/blob/master/src/sysv/SUSE/sssd.in somehow?

joakim-tjernlund · 2021-04-06T10:04:49Z

This PR forgot to change -f in src/sysv/gentoo/sssd.in so sssd now fails to start as -f option is not recognized.
Please replace -f with --logger=files

@joakim-tjernlund , do we really need distribution-specific config in upstream repo?

@scabrero , do you use https://github.com/SSSD/sssd/blob/master/src/sysv/SUSE/sssd.in somehow?

Hi, I don't think we need dist. specific ones but there are variables in there that needs processing.
Also, Gentoo vs. SUSE, the Gentoo one is openrc while SUSE is a plain script so you cannot merge them into one.

joakim-tjernlund · 2021-04-06T10:12:29Z

To be clear, you could have a generic openrc script and a generic SYSV shell script

alexey-tikhonov · 2021-04-06T10:18:06Z

Hi, I don't think we need dist. specific ones but there are variables in there that needs processing.

Is this file really used in Gentoo?
There are:
https://gitweb.gentoo.org/repo/gentoo.git/tree/sys-auth/sssd/files/sssd.service
https://gitweb.gentoo.org/repo/gentoo.git/tree/sys-auth/sssd/files/sssd

But I'm really not familiar with Gentoo packaging thus asking.

To be clear, you could have a generic openrc script and a generic SYSV shell script

Yes, this makes sense.

joakim-tjernlund · 2021-04-06T10:26:20Z

Hi, I don't think we need dist. specific ones but there are variables in there that needs processing.

Is this file really used in Gentoo?
There are:
https://gitweb.gentoo.org/repo/gentoo.git/tree/sys-auth/sssd/files/sssd.service
https://gitweb.gentoo.org/repo/gentoo.git/tree/sys-auth/sssd/files/sssd

But I'm really not familiar with Gentoo packaging thus asking.

Yes, you can choose systemd or openrc at build time(Gentoo is a source based dist so every SW pkg are built locally)
The sssd.service is just used in old 2.2.0. Same for sssd file.

alexey-tikhonov · 2021-04-06T10:36:17Z

Ok, so probably we could rename sysv/gentoo/sssd.in to sysv/openrc/sssd.in and get rid of sysv/SUSE/ (merge to sysv/sssd.in)...

scabrero · 2021-04-06T13:50:12Z

@scabrero , do you use https://github.com/SSSD/sssd/blob/master/src/sysv/SUSE/sssd.in somehow?

No we don't use it. We are using src/sysv/systemd/*.

see SSSD#5535 (comment)

alexey-tikhonov · 2021-04-06T15:30:04Z

#5569

see SSSD#5535 (comment)

see #5535 (comment) Reviewed-by: Pavel Březina <pbrezina@redhat.com>

see SSSD#5535 (comment) Reviewed-by: Pavel Březina <pbrezina@redhat.com>

alexey-tikhonov linked an issue Mar 12, 2021 that may be closed by this pull request

Unexpected (?) side effect of SSSDBG_DEFAULT change #5488

Closed

alexey-tikhonov added the Waiting for review label Mar 15, 2021

alexey-tikhonov added 11 commits March 16, 2021 13:29

Moved ldb_debug_messages() out of UTILS to SYSDB

c94ccdb

Moved declaration of debug related helpers

d672ba4

defined in debug.c from util.h to debug.h

DEBUG: make use of existing SSSD_DEBUG_OPTS macro

83cf4ec

DEBUG: incorporate sss_set_logger() into DEBUG_INIT

753c770

This makes code less error-prone reducing amount of function calls required for debug initialization.

DEBUG: remove sss_set_logger() from public API

506edcd

DEBUG: added several comments to debug.h API

8957212

and moved rarely used / "private" functions to the bottom.

Moved SSSDBG_MASK_ALL out of debug.h

1390251

since is it is only used in tests.

DEBUG: incorporate open_debug_file() into DEBUG_INIT

c7a3a49

This makes code less error-prone reducing amount of function calls required for debug initialization.

MONITOR: added logging of cmd used to start services

72fdfc5

DEBUG: introduce SSSDBG_TOOLS_DEFAULT

5dcd0dc

Resolves: SSSD#5488

alexey-tikhonov force-pushed the logs_sanitize branch from aad2ebd to 5dcd0dc Compare March 16, 2021 14:03

alexey-tikhonov requested review from sumit-bose, thalman and pbrezina and removed request for thalman, sumit-bose and pbrezina March 18, 2021 15:02

alexey-tikhonov mentioned this pull request Mar 24, 2021

Unexpected (?) side effect of SSSDBG_DEFAULT change #5488

Closed

thalman self-assigned this Mar 24, 2021

MONITOR: in case '-i' is given don't force logger to 'stderr'

0ff5961

if its value specified explictly

thalman added Accepted and removed Waiting for review labels Mar 31, 2021

pbrezina added the Ready to push Ready to push label Apr 1, 2021

pbrezina added Pushed and removed Accepted Ready to push Ready to push labels Apr 1, 2021

pbrezina closed this Apr 1, 2021

alexey-tikhonov added a commit to alexey-tikhonov/sssd that referenced this pull request Apr 6, 2021

SYSV: removed unused SUSE/sssd.id

077b904

see SSSD#5535 (comment)

alexey-tikhonov added a commit to alexey-tikhonov/sssd that referenced this pull request Apr 6, 2021

SYSV: removed unused SUSE/sssd.id

32b6381

see SSSD#5535 (comment)

pbrezina pushed a commit that referenced this pull request Apr 13, 2021

SYSV: removed unused SUSE/sssd.id

dab0ead

see #5535 (comment) Reviewed-by: Pavel Březina <pbrezina@redhat.com>

akuster pushed a commit to akuster/sssd that referenced this pull request May 18, 2021

SYSV: removed unused SUSE/sssd.id

37b7d27

see SSSD#5535 (comment) Reviewed-by: Pavel Březina <pbrezina@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A set of patches to sanitize logger code a little bit. #5535

A set of patches to sanitize logger code a little bit. #5535

alexey-tikhonov commented Mar 12, 2021 •

edited

Loading

thalman commented Mar 24, 2021

thalman commented Mar 31, 2021

alexey-tikhonov commented Mar 31, 2021

thalman commented Mar 31, 2021

pbrezina commented Apr 1, 2021

joakim-tjernlund commented Apr 2, 2021

alexey-tikhonov commented Apr 6, 2021

joakim-tjernlund commented Apr 6, 2021

joakim-tjernlund commented Apr 6, 2021

alexey-tikhonov commented Apr 6, 2021

joakim-tjernlund commented Apr 6, 2021

alexey-tikhonov commented Apr 6, 2021

scabrero commented Apr 6, 2021

alexey-tikhonov commented Apr 6, 2021

A set of patches to sanitize logger code a little bit. #5535

A set of patches to sanitize logger code a little bit. #5535

Conversation

alexey-tikhonov commented Mar 12, 2021 • edited Loading

thalman commented Mar 24, 2021

thalman commented Mar 31, 2021

alexey-tikhonov commented Mar 31, 2021

thalman commented Mar 31, 2021

pbrezina commented Apr 1, 2021

joakim-tjernlund commented Apr 2, 2021

alexey-tikhonov commented Apr 6, 2021

joakim-tjernlund commented Apr 6, 2021

joakim-tjernlund commented Apr 6, 2021

alexey-tikhonov commented Apr 6, 2021

joakim-tjernlund commented Apr 6, 2021

alexey-tikhonov commented Apr 6, 2021

scabrero commented Apr 6, 2021

alexey-tikhonov commented Apr 6, 2021

alexey-tikhonov commented Mar 12, 2021 •

edited

Loading