function names in multi-task scenarios
by Yassir Elley
In looking through the sssd code, there are instances where an asynchronous operation requires multiple tasks to be done in series (not to be confused with a parent request with multiple child requests). An example of a series of such tasks would look like:
* send()
* task1()
* task2()
* done()
While one would expect that the tasks would be named so as to describe their behavior, they are typically named as completing a previous task, giving no indication what the task is going to do. This can be confusing when reading the logs. For example, if the problem is that kinit failed, an entry in the log might have the function name "sdap_cli_rootdse_done", which doesn't help describe the kinit problem (the call stack below shows how "sdap_cli_rootdse_done" calls "sdap_cli_kinit_step").
In the following call stack, I have used the "bad name" annotation for those function names that don't describe the function behavior. I have also suggested using a more descriptive name that describes the functions behavior (each using a <suffix>). I'm not sure what the <suffix> should be; maybe "task" or "phase" or something.
sdap_cli_connect_send
sdap_cli_resolve_next
sdap_cli_resolve_done bad name; sdap_cli_connect_<suffix> would be better (maybe sdap_cli_connect_task)
sdap_connect_send
sdap_cli_connect_done bad_name; sdap_cli_rootdse_<suffix> would be better
sdap_cli_rootdse_step
sdap_cli_rootdse_done bad_name; sdap_cli_kinit_<suffix> would be better
sdap_cli_kinit_step
sdap_cli_kinit_done bad_name; sdap_cli_auth_<suffix> would be better
sdap_cli_auth_step
sdap_cli_auth_done bad_name; sdap_cli_connect_done would be better
tevent_req_done
In addition to being non-descriptive, the "done" suffix is currently used in two different ways, which can be confusing:
* as shown above, "done" is used for a series of intermediate tasks, whose names are based on previous tasks
* in a simpler scenario (with no intermediate tasks), "done" is used to signify the final task (which makes sense); for example:
sdap_id_op_connect_send
sdap_id_op_connect_step
sdap_id_op_connect_done
To resolve both issues, I propose we improve our naming conventions by:
* coming to consensus on a suffix for a series of intermediate tasks
* declaring "done" to be used only for a final task (whether in a basic scenario, or a scenario with multiple intermediate tasks)
What do others think?
Regards,
Yassir.
10 years, 3 months
ding-libs unit test
by Dmitri Pal
Hello,
Yesterday I rebased my repo on top of the latest patches for ding-libs.
My make check runs fine but if I then run the test manually it breaks.
I have seen this in the In the past. Something changed in how the build
works and now causes this.
I need to investigate. I will try to resolve it before Monday, stay tuned.
--
Thank you,
Dmitri Pal
Sr. Engineering Manager for IdM portfolio
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
10 years, 3 months
[PATCH] Inheric home dir and shell overrides for subdomains
by Jakub Hrozek
Hi,
Jeremy Agee from our QE team found out an inconsistency in how we handle
the home directory and shell fallbacks. If the fallback is set globally
in the [nss] section, then everything works as expected and the
fallbacks are honoured for all users. However, if the fallbacks are set
in the [domain] section, then they only apply for the parent domain, not
subdomains.
I think there is no reason to not inherit the overrides. Even though
there is separate subdomain_homedir override, that parameter serves a
different use case when the primary domain (typically IPA) has POSIX
attributes, but the subdomain does not.
For AD case, I think we should just inherit these settings when creating
the subdomain. The IPA server mode would use these values and they can
be picked up by legacy clients. Non-legacy clients do not have a way to
use the homedir and shell discovered on the server as the extop only
transmits name, UID and GID.
The other patch that is attached is meant to support the default homedir
expansion that contains user name. But because subdomain users have the
FQDN in the name attribute, the 'plain' username has to be extracted from
the FQDN until Michal finishes his refactoring.
10 years, 3 months
[PATCH] TESTS: Fix build with older version of check framework
by Lukas Slebodnik
ehlo,
building of sssd fail on platforms with older version of check unit testing
framework (0.9.8)
src/tests/safe-format-tests.c: In function 'test_safe_format_string_cb':
src/tests/safe-format-tests.c:158: error: implicit declaration of function 'ck_assert_int_ge'
src/tests/safe-format-tests.c:162: error: implicit declaration of function 'ck_assert_int_lt'
make[2]: *** [src/tests/safe_format_tests-safe-format-tests.o] Error 1
make[2]: *** Waiting for unfinished jobs....
patch attached.
LS
10 years, 3 months
[PATCH] Don't pass user input as a printf format string argument
by Stef Walter
full_name_format is documented as a printf(3) compatible format string,
and the util.c implementation passes that string to snprintf after some
sneaky hacks to make it actually work ...
It's not good practice to pass user input (even admin input) to printf
directly. Doing so leads to crashes and abort()'s when the format string
isn't just right. This probably isn't exploitable, but as usual it's
hard to be sure :)
Fedora and others check for sorta thing with -Wformat-security flags.
Unlike regular expressions, printf format strings aren't meant for user
provided input. There are probably better choices for the
full_name_format syntax. I think any work on that should be separate
from this patch.
Anyhow, here's a patch which aims to make the full_name_format printf
handling both correct and safe.
This patch includes safe-printf.[ch] from the realmd project, which are
written with just standard libc deps, and uses it for formatting fully
qualified names. [1]
In addition there were various corner cases and assumptions about the
format strings, such as the exact syntax of the args, the length of the
output, etc... which I've fixed in this patch.
Tests are added and updated.
I'll be happy to split the patch into two, if desired. One which adds
safe-printf.[ch] + tests, and the second which fixes the fully qualified
names.
I believe that this patch also simplifies fqname handling code.
Cheers,
Stef
[1] realmd currently uses full_name_format to build it's LoginFormats
property. Once sssd is on the system DBus bus, realmd might be able to
ask sssd for this directly...
10 years, 3 months
[INI][PATCHES] All of them (C-Style Comments, Unicode conversion, Space processing, UNIT tests)
by Dmitri Pal
Hello,
This mail contains all the patches that I have so far in my git.
--------------------------
The following set of patches is related to comments and unchanged
from the previous submission.
I replied to the comments about them in another mail.
I do not think there is anything to address.
If there is please let me know.
0001-INI-Do-not-check-validity-of-comments.patch
0002-INI-Extend-error-set-and-add-parsing-error.patch
0003-INI-Process-c-style-comments.patch
0004-INI-Test-files-for-unit-test.patch
0005-INI-Unit-test-for-c-style-comments.patch
--------------------------
The following set of patches addresses comments and fixes
bugs in Unicode/UTF8 conversion
0006-INI-Expose-buffer-context-as-void.patch
0007-INI-Extend-internal-file-handle.patch
0008-INI-Convert-files-to-UTF.patch
0009-INI-Updated-unit-test-for-UTF8-conversion.patch
Thanks for review Lukas. I reworked the code significantly.
I think I addressed everything except the initialization comment.
I agree with you that it would have been better to
initialize things outside the loop but I need to read the first buffer
to be able to determine the encoding. This is what causes this logic.
I do not see a way to avoid it so left that part as is.
----------------------------
The following one liner fixes a trace text
0010-INI-Fix-typo-in-comment.patch
----------------------------
The following patches are related to the problem I noticed earlier
and recorded in the ticket https://fedorahosted.org/sssd/ticket/2119
But in reality the cause was bug in the Unicode conversion
and a bug with space processing.
0011-INI-Fix-processing-of-the-white-space-at-the-end-of-.patch
0012-INI-Unit-test-for-space-trimming-in-multiline-values.patch
----------------------------
The following patch adds more unit test for both Unicode conversion
and folding by combining the tests together. The unit tests written
for each of them individually are not sufficient to catch corner cases.
This test does. And I played with different constants before checking
in to make sure that all sorts of combinations are caught.
I decided to settle on these numbers because they provide
sufficient amount of testing but do not extend the time too much.
Right now all make check tests take about 7-8 seconds on my laptop.
0013-INI-Adding-more-unit-tests.patch
--
Thank you,
Dmitri Pal
Sr. Engineering Manager for IdM portfolio
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
10 years, 3 months
[PATCH] Warnings and -Werror from ./configure
by Stef Walter
Hey guys,
The other day Stephen suggested that instead of complaining (heh heh) I
should submit some patches for moving some of bashrc_sssd into
./configure to make stuff in there more useful.
Here we are.
Do these changes need to get squirreled away into an m4 file? If so
which one?
Patch 0002 adds --enable-strict to drive -Werror. Is there anything else
it should drive?
Does sssd not use AM_MAINTAINER_MODE([enable]) by choice? If it did use
--enable-maintainer-mode then that could provide the --enable-strict
defaults.
Cheers,
Stef
10 years, 3 months
[PATCH] util: Fix const cast failures when building with -Werror
by Stef Walter
This results when running 'make check' with $SSS_ERROR set:
../src/tests/cmocka/test_authtok.c: In function ‘test_sss_authtok_password’:
../src/tests/cmocka/test_authtok.c:98:48: error: cast discards
‘__attribute__((const))’ qualifier from pointer target type
[-Werror=cast-qual]
ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
^
../src/tests/cmocka/test_authtok.c: In function ‘test_sss_authtok_ccfile’:
../src/tests/cmocka/test_authtok.c:137:48: error: cast discards
‘__attribute__((const))’ qualifier from pointer target type
[-Werror=cast-qual]
ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
^
../src/tests/cmocka/test_authtok.c:161:48: error: cast discards
‘__attribute__((const))’ qualifier from pointer target type
[-Werror=cast-qual]
ret = sss_authtok_set(ts->authtoken, type, (uint8_t *) data, 0);
^
../src/tests/cmocka/test_authtok.c: In function
‘test_sss_authtok_wipe_password’:
../src/tests/cmocka/test_authtok.c:239:48: error: cast discards
‘__attribute__((const))’ qualifier from pointer target type
[-Werror=cast-qual]
ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
^
../src/tests/cmocka/test_authtok.c: In function ‘test_sss_authtok_copy’:
../src/tests/cmocka/test_authtok.c:271:48: error: cast discards
‘__attribute__((const))’ qualifier from pointer target type
[-Werror=cast-qual]
ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
^
../src/tests/cmocka/test_authtok.c:278:48: error: cast discards
‘__attribute__((const))’ qualifier from pointer target type
[-Werror=cast-qual]
ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
^
Cheers,
Stef
10 years, 3 months