Skip to content

Unifying boilerplate code for handling child processes - part 2#8174

Merged
alexey-tikhonov merged 17 commits into
SSSD:masterfrom
alexey-tikhonov:child-helpers-2
Nov 21, 2025
Merged

Unifying boilerplate code for handling child processes - part 2#8174
alexey-tikhonov merged 17 commits into
SSSD:masterfrom
alexey-tikhonov:child-helpers-2

Conversation

@alexey-tikhonov

@alexey-tikhonov alexey-tikhonov commented Nov 7, 2025

Copy link
Copy Markdown
Member

Follow up of #7991

@alexey-tikhonov alexey-tikhonov added the no-backport This should go to target branch only. label Nov 7, 2025
@alexey-tikhonov
alexey-tikhonov force-pushed the child-helpers-2 branch 3 times, most recently from 8895fee to 337d467 Compare November 13, 2025 13:26
@alexey-tikhonov
alexey-tikhonov marked this pull request as ready for review November 13, 2025 13:26
@alexey-tikhonov alexey-tikhonov added coverity Trigger a coverity scan and removed coverity Trigger a coverity scan labels Nov 13, 2025
@alexey-tikhonov

alexey-tikhonov commented Nov 13, 2025

Copy link
Copy Markdown
Member Author

Covscan complains

332    if (getresgid(&rgid, &egid, &sgid) == 0) {
     
CID 638588: (#1 of 1): Unchecked return value (CHECKED_RETURN)
18. check_return: Calling setresgid without checking return value (as is done elsewhere 4 out of 5 times).
333        setresgid(sgid, sgid, sgid);

but this PR doesn't touch this code. Weird.

No other issues reported.

@alexey-tikhonov

alexey-tikhonov commented Nov 14, 2025

Copy link
Copy Markdown
Member Author

(rebased)

@alexey-tikhonov

alexey-tikhonov commented Nov 14, 2025

Copy link
Copy Markdown
Member Author

Ok, at least this hang on

tests/test_identity.py::test_identity__lookup_group_gid_with_getent (ldap) FAILED [ 47%]

isn't introduced by this PR.

https://github.com/SSSD/sssd/actions/runs/19366604432/job/55412465094?pr=8193 has the same issue.

@alexey-tikhonov alexey-tikhonov mentioned this pull request Nov 15, 2025
Merged
@alexey-tikhonov

alexey-tikhonov commented Nov 18, 2025

Copy link
Copy Markdown
Member Author

(rebased)

thalman
thalman requested changes Nov 19, 2025
View reviewed changes

@thalman thalman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks @alexey-tikhonov,

See my one nitpicking comment, sorry for that ;-)

Comment thread src/util/child_bootstrap.h Outdated
SSSD_DEBUG_OPTS \
SSSD_LOGGER_OPTS(&sss_child_basic_settings.opt_logger) \
{"dumpable", 0, POPT_ARG_INT, &sss_child_basic_settings.dumpable, 0, \
sss_child_basic_settings.ignore_dumpable ? _("Ignored") : _("Allow core dumps"), NULL }, \

@thalman thalman Nov 19, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just nitpicking, can you remove some extra spaces before trailing \?

@alexey-tikhonov alexey-tikhonov Nov 19, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@thalman

thalman commented Nov 19, 2025

Copy link
Copy Markdown
Contributor

There was some github outage yesterday, This is why CI is so red.

thalman
thalman approved these changes Nov 20, 2025
View reviewed changes

@thalman thalman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, ACK

pbrezina
pbrezina approved these changes Nov 21, 2025
View reviewed changes

@pbrezina pbrezina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-running PR CI but ack. Nice job!

@alexey-tikhonov @sssd-bot
Get rid of useless SSSD_MAIN_OPTS define.
101c261 
Reviewed-by: Pavel Březina <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
@alexey-tikhonov @sssd-bot
Makefile: tools do not need to link against 'sss_client' code
7ea9518 
Reviewed-by: Pavel Březina <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
@alexey-tikhonov @sssd-bot
Makefile: get rid of useless 'SSSD_LCL_TOOLS_OBJ'
482c8ad 
Reviewed-by: Pavel Březina <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
@alexey-tikhonov @sssd-bot
Move 'DEBUG_CHAIN_ID_FMT_*' from 'util.h'
4418cae 
to 'sss_chain_id.h' where it makes a better sense.

Reviewed-by: Pavel Březina <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
@alexey-tikhonov @sssd-bot
Include <libintl.h> in 'debug.h'
b39bad3 
Since this header uses `gettext()`

Reviewed-by: Pavel Březina <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
@alexey-tikhonov @sssd-bot
OIDC_CHILD: use DEBUG_CHAIN_ID_FMT_RID
c89f363 
instead of DEBUG_CHAIN_ID_FMT_CID since this process is invoked
by a provider.

Reviewed-by: Pavel Březina <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
@alexey-tikhonov @sssd-bot
Helpers to do a basic setup of a child process.
f64ec79 
Plus an application in 'ldap_child'.

Reviewed-by: Pavel Březina <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
@alexey-tikhonov @sssd-bot
KRB5_CHILD: use new helper to setup a process
05eee4a 
Reviewed-by: Pavel Březina <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
@alexey-tikhonov @sssd-bot
OIDC_CHILD: use new helper to setup a process
3250cbe 
Reviewed-by: Pavel Březina <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
@alexey-tikhonov @sssd-bot
AD_GPO_CHILD: use new helper to setup a process
ff85ab9 
Reviewed-by: Pavel Březina <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
@alexey-tikhonov @sssd-bot
P11_CHILD: use new helper to setup a process
025ad2a 
Reviewed-by: Pavel Březina <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
@alexey-tikhonov @sssd-bot
PASSKEY_CHILD: use new helper to setup a process
903c346 
This also makes 'passkey_child' to use '--chain-id' if supplied.

Reviewed-by: Pavel Březina <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
@alexey-tikhonov @sssd-bot
SELINUX_CHILD: use new helper to setup a process
f9acabd 
Reviewed-by: Pavel Březina <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
@alexey-tikhonov @sssd-bot
SELINUX_CHILD: fix includes
89780cb 
Reviewed-by: Pavel Březina <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
@alexey-tikhonov @sssd-bot
DUMMY_CHILD TEST: use new helper to setup a process
d850bf7 
Reviewed-by: Pavel Březina <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
@alexey-tikhonov @sssd-bot
DEBUG: use 'debug_prg_name' if 'debug_log_file' isn't set
89c3556 
This avoids using 'sssd' potentially by multiple binaries.

Reviewed-by: Pavel Březina <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
@alexey-tikhonov @sssd-bot
PROXY_CHILD: use new helper to setup a process
1b5e7ae 
Reviewed-by: Pavel Březina <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
@sssd-bot

sssd-bot commented Nov 21, 2025

Copy link
Copy Markdown
Contributor

The pull request was accepted by @pbrezina with the following PR CI status:

🟢 CodeFactor (success)
🟢 CodeQL (success)
NEUTRAL osh-diff-scan:fedora-rawhide-x86_64:upstream (neutral)
🟢 rpm-build:centos-stream-10-x86_64:upstream (success)
🟢 rpm-build:fedora-41-x86_64:upstream (success)
🟢 rpm-build:fedora-42-x86_64:upstream (success)
🟢 rpm-build:fedora-43-x86_64:upstream (success)
🟢 rpm-build:fedora-rawhide-x86_64:upstream (success)
🟢 Analyze (target) / cppcheck (success)
🟢 Build / freebsd (success)
🟢 Build / make-distcheck (success)
🟢 ci / intgcheck (centos-10) (success)
🟢 ci / intgcheck (fedora-41) (success)
🟢 ci / intgcheck (fedora-42) (success)
🟢 ci / intgcheck (fedora-43) (success)
🟢 ci / intgcheck (fedora-44) (success)
🟢 ci / prepare (success)
🟡 ci / system (centos-10) (in_progress)
🟢 ci / system (fedora-41) (success)
🟢 ci / system (fedora-42) (success)
🟡 ci / system (fedora-43) (in_progress)
🟡 ci / system (fedora-44) (in_progress)
➖ Coverity scan / coverity (skipped)
🟢 Static code analysis / codeql (success)
🟢 Static code analysis / pre-commit (success)
🟢 Static code analysis / python-system-tests (success)

There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging.

@alexey-tikhonov
alexey-tikhonov merged commit 6aa7c9a into SSSD:master Nov 21, 2025
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thalman thalman thalman approved these changes
@pbrezina pbrezina pbrezina approved these changes

Assignees

@thalman thalman

@pbrezina pbrezina

Labels

Accepted no-backport This should go to target branch only.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@alexey-tikhonov @thalman @sssd-bot @pbrezina