On (14/07/15 13:21), Petr Cech wrote: >On 07/13/2015 07:13 PM, Lukas Slebodnik wrote: >>On (13/07/15 10:57), Jakub Hrozek wrote: >>>On Mon, Jul 13, 2015 at 09:47:46AM +0200, Lukas Slebodnik wrote: >>>>On (10/07/15 16:54), Jakub Hrozek wrote: >>>>>On Wed, Jul 08, 2015 at 03:26:52PM +0200, Sumit Bose wrote: >>>>>> 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. >>>>>This is really important, so much that I wonder if we should move all >>>>>the files that are used by both client code and daemon code to some new >>>>>directory in the SSSD tree (src/shared/ maybe) and use a different comment >>>>>header in these files. >>>>We do not need to use sss_cmd2str in client code. >>>>If you wan to see debug messages from pam_sss module then you >>>>need to recompile source code with extra CFLAG to enable them. >>>Good point. >>> >>>>It very unlikely that debug messages in pam_sss code will used by users. >>>>I would prefer do not touch client code or used just hexadecimal >>>>represaentation (the same as in header file) >>>I agree, let's not touch the client unless needed. >>Another reason for not using sss_cmd2str in client code is that >>it depends on our "debug_fn" from internal library libsss_debug. >> >>Even thought the function sss_cmd2str was not used in pam_sss.c >>it was still linked with pam_sss.so and thus dlopen test failed. >>Petr already noticed it; This mail is just summary of off the list >>discussion. >> >>LS > >Hi, > >there is another repaired patch. >Changes are: >* hexadecimal numbers instead of cmd2str() in sss_client, >* added license preamble in headers of new files. >
I would like to appolozie for late review. I thought somebody else will continue. >Andthere is a comment of Lukas Slebodnik for that I need more investigation. >> BTW It would be good to use new function also in backend code. >> src/providers/data_provider_be.c:1107: "Got request for >[%#x][%d][%s]\n", type, attr_type, filter); >> I used to filter debug messages for be_get_account_info which print >> type as hexadecimal number. Maybe there are also other places. >> LS > >Petr >From a93e36f11759cf9a748942e7632d4a07a088b098 Mon Sep 17 00:00:00 2001 >From: Petr Cech <[email protected]> >Date: Wed, 8 Jul 2015 07:17:28 -0400 >Subject: [PATCH] UTIL: Function 2string for enum sss_cli_command > >Improvement of debug messages. >Instead of:"(0x0400): Running command [17]..." >We could see:"(0x0400): Running command [17][SSS_NSS_GETPWNAM]..." >(It's not used in sss_client. There are only hex numbers of commands.) > >Resolves: >https://fedorahosted.org/sssd/ticket/2708 The patch does not apply to master. I had to use tree way merge. Please rebase it. >--- > Makefile.am | 3 +- > src/providers/dp_pam_data_util.c | 27 +---- > src/responder/nss/nsssrv_cmd.c | 30 ++--- > src/sss_client/pam_sss.c | 6 +- > src/tools/tools_mc_util.c | 4 +- > src/util/sss_cli_cmd.c | 238 +++++++++++++++++++++++++++++++++++++++ > src/util/sss_cli_cmd.h | 28 +++++ > 7 files changed, 293 insertions(+), 43 deletions(-) > create mode 100644 src/util/sss_cli_cmd.c > create mode 100644 src/util/sss_cli_cmd.h > >diff --git a/Makefile.am b/Makefile.am >index >b8cbc6df23ded1edb945a709b6dbe1c44eb54017..430f2292a1be9e0f0b7cb56e8ecbf179e9978dcd > 100644 >--- a/Makefile.am >+++ b/Makefile.am >@@ -678,7 +678,8 @@ endif > pkglib_LTLIBRARIES += libsss_debug.la > libsss_debug_la_SOURCES = \ > src/util/debug.c \ >- src/util/sss_log.c >+ src/util/sss_log.c \ >+ src/util/sss_cli_cmd.c We decided to add "$NULL" at the end of list so in future you will not need to change two lines if you add new file. > libsss_debug_la_LIBADD = \ > $(SYSLOG_LIBS) > libsss_debug_la_LDFLAGS = \ >diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c >index >0129467302f16af318bbbb0a5be47ff2e235da65..d37a13820ef857fcf43efba3fb07535c4b6eb509 > 100644 >--- a/src/responder/nss/nsssrv_cmd.c >+++ b/src/responder/nss/nsssrv_cmd.c >@@ -1656,7 +1656,7 @@ static int pam_sss(enum sss_cli_command task, >pam_handle_t *pamh, > case SSS_PAM_CLOSE_SESSION: > break; > default: >- D(("Illegal task [%d]", task)); >+ D(("Illegal task [%#x]",task)); ^ There was a space before change. Could you return it back. > return PAM_SYSTEM_ERR; > } >diff --git a/src/util/sss_cli_cmd.c b/src/util/sss_cli_cmd.c >new file mode 100644 >index >0000000000000000000000000000000000000000..97b967b4014193dc8f7571e5fe821523d469f201 >--- /dev/null >+++ b/src/util/sss_cli_cmd.c >@@ -0,0 +1,238 @@ >+/* >+ SSSD - cmd2str util >+ >+ Copyright (C) Petr Cech <[email protected]> 2015 >+ >+ This program is free software; you can redistribute it and/or modify >+ it under the terms of the GNU General Public License as published by >+ the Free Software Foundation; either version 3 of the License, or >+ (at your option) any later version. >+ >+ This program is distributed in the hope that it will be useful, >+ but WITHOUT ANY WARRANTY; without even the implied warranty of >+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >+ GNU General Public License for more details. >+ >+ You should have received a copy of the GNU General Public License >+ along with this program. If not, see <http://www.gnu.org/licenses/>. >+*/ >+ >+#include "sss_client/sss_cli.h" >+#include "util/sss_cli_cmd.h" >+#include "util/util.h" >+ >+const char *sss_cmd2str(enum sss_cli_command cmd) >+{ //snip >+ >+ #if 0 >+ /* shadow */ >+ case SSS_NSS_GETSPNAM: >+ return "SSS_NSS_GETSPNAM"; >+ case SSS_NSS_GETSPUID: >+ return "SSS_NSS_GETSPUID"; >+ case SSS_NSS_SETSPENT: >+ return "SSS_NSS_SETSPENT"; >+ case SSS_NSS_GETSPENT: >+ return "SSS_NSS_GETSPENT"; >+ case SSS_NSS_ENDSPENT: >+ return "SSS_NSS_ENDSPENT"; >+ #endif I think it's better to be consistent with header file and we can have unused options here. But it's better to do not add spaces before '#' I saw a patter in some header files that spaces was added after this character. Something like #if defined __GNUC__ # if defined __NO_INLINE__ # define HAVE_INLINE 0 # else # define HAVE_INLINE 1 # ifndef inline # define inline __inline__ # endif # endif #elif defined __cplusplus Please remove indentation for "#if" and "#endif" in whole file. >+ >+ /* SUDO */ >+ case SSS_SUDO_GET_SUDORULES: >+ return "SSS_SUDO_GET_SUDORULES"; >+ case SSS_SUDO_GET_DEFAULTS: >+ return "SSS_SUDO_GET_DEFAULTS"; >+ //snip >+ >+ /* ID-SID mapping calls */ >+ case SSS_NSS_GETSIDBYNAME: >+ return "SSS_NSS_GETSIDBYNAME"; >+ case SSS_NSS_GETSIDBYID: >+ return "SSS_NSS_GETSIDBYID"; >+ case SSS_NSS_GETNAMEBYSID: >+ return "SSS_NSS_GETNAMEBYSID"; >+ case SSS_NSS_GETIDBYSID: >+ return "SSS_NSS_GETIDBYSID"; >+ case SSS_NSS_GETORIGBYNAME: >+ return "SSS_NSS_GETORIGBYNAME"; >+ default: >+ DEBUG(SSSDBG_MINOR_FAILURE, >+ "Translation's string is missing for command [%#x].\n", cmd); >+ return "UNKNOWN COMMAND"; >+ } ^^^ Just thressspaces here :-) Otherwise LGTM LS _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
