[SSSD] [PATCH] UTIL: Function 2string for enum sss_cli_command

Lukas Slebodnik lslebodn at redhat.com
Thu Aug 13 09:11:46 UTC 2015


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 <pcech at redhat.com>
>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 <pcech at redhat.com> 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



More information about the sssd-devel mailing list