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(a)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(a)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