On Wed, Jul 08, 2015 at 02:13:42PM +0200, Petr Cech wrote:
Hi!
https://fedorahosted.org/sssd/ticket/2703
It's my first patch to this ticket. It is simple transforming of number of
command to the string.
Hi Petr,
welcome and thank you for your first patch. Besides Pavel's suggestions
I have some general comments as well.
- There is pamcmd2str() which does a similar job for the backend code
but I think it is becoming redundant with your patch. Can you remove
this call and use your's where appropriate?
- I haven't tested it, but I'm pretty sure that the PAM module pam_sss
which is build from pam_sss.c and some other files is broken in debug
mode with your patch because sss_log.c is not used when building it and
hence sss_cli_command_2string() will be undefined. You do not see this
during compilation or even during 'make check' because the 'D' macro
is only evaluate if PAM_DEBUG is defined during compilation. If you
run something like 'make CFLAGS+="-DPAM_DEBUG" check' the dlopen test
should fail with your patch.
Since the PAM module pam_sss.so might be loaded by any kind of
processes at runtime we try to keep it as simple as possible and try
to add as few dependencies as possible. If you search the Makefile.am
for pam_sss_la_SOURCES you will see that besides source files from the
sss_client directory we only add atomic_io.c and authtok-utils.c which
both contain only a single function with no special dependencies.
I would suggest that you put sss_cli_command_2string() in a file on
its own similar like atomic_io.c or authtok-utils.c. And add this file
to pam_sss_la_SOURCES and libsss_debug_la_SOURCES in Makefile.am. I
leave it up to you to decide what would be a good place for this file.
The sss_client directory because the enum sss_cli_command is defined
here as well or the util directory because the main usage for it is in
the SSSD code and not in the pam_sss module.
bye,
Sumit