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.
Petr
On 07/08/2015 02:13 PM, 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.
Petr
Hello Petr,
thank for the patch!
I haven't tested the patch yet, but I have some nitpicks. Please see inline.
0001-UTIL-Function-2string-for-enum-sss_cli_command.patch
From ca782a1518480635ef60bc2cdf77d9b8644132b0 Mon Sep 17 00:00:00 2001 From: Petr Cechpcech@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] with..." We could see:"(0x0400): Running command [SSS_NSS_GETPWNAM] with..."
Is this the right ticket? Subject of the ticket is "Need better libhbac debuging added to sssd" but I don't think this patch relates to libhbac, right?
src/responder/nss/nsssrv_cmd.c | 29 +++--- src/sss_client/pam_sss.c | 7 +- src/tools/tools_mc_util.c | 4 +- src/util/sss_log.c | 215 +++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 4 + 5 files changed, 242 insertions(+), 17 deletions(-)
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c index 0129467302f16af318bbbb0a5be47ff2e235da65..421048ec71891b87f6be08efe41fa7c48c97cfaa 100644 --- a/src/responder/nss/nsssrv_cmd.c +++ b/src/responder/nss/nsssrv_cmd.c @@ -1312,7 +1312,8 @@ static int nss_cmd_getbynam(enum sss_cli_command cmd, struct cli_ctx *cctx) case SSS_NSS_GETORIGBYNAME: break; default:
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command type [%d].\n", cmd);
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command type [%s].\n",
sss_cli_command_2string(cmd)); return EINVAL; }
I think that removing the number value might be a little too much. There might be people used to it. So I think that showing both might be the best option. Please see how we use 'sss_strerror' which is similar to your function. Are there other opinions?
@@ -1347,8 +1348,8 @@ static int nss_cmd_getbynam(enum sss_cli_command cmd, struct cli_ctx *cctx)
rawname = (const char *)body;
- DEBUG(SSSDBG_TRACE_FUNC, "Running command [%d] with input [%s].\n",
dctx->cmdctx->cmd, rawname);
DEBUG(SSSDBG_TRACE_FUNC, "Running command [%s] with input [%s].\n",
sss_cli_command_2string(dctx->cmdctx->cmd), rawname); if (dctx->cmdctx->cmd == SSS_NSS_GETSIDBYNAME) { ret = nss_check_name_of_well_known_sid(cmdctx, rawname);
@@ -1737,7 +1738,8 @@ static int nss_cmd_getbyid(enum sss_cli_command cmd, struct cli_ctx *cctx) case SSS_NSS_GETSIDBYID: break; default:
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command type [%d].\n", cmd);
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command type [%s].\n",
sss_cli_command_2string(cmd)); return EINVAL; }
@@ -1766,8 +1768,8 @@ static int nss_cmd_getbyid(enum sss_cli_command cmd, struct cli_ctx *cctx) } SAFEALIGN_COPY_UINT32(&cmdctx->id, body, NULL);
- DEBUG(SSSDBG_TRACE_FUNC, "Running command [%d] with id [%"PRIu32"].\n",
dctx->cmdctx->cmd, cmdctx->id);
DEBUG(SSSDBG_TRACE_FUNC, "Running command [%s] with id [%"PRIu32"].\n",
sss_cli_command_2string(dctx->cmdctx->cmd), cmdctx->id); switch(dctx->cmdctx->cmd) { case SSS_NSS_GETPWUID:
@@ -1805,8 +1807,8 @@ static int nss_cmd_getbyid(enum sss_cli_command cmd, struct cli_ctx *cctx) } break; default:
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command [%d].\n",
dctx->cmdctx->cmd);
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command [%s].\n",
sss_cli_command_2string(dctx->cmdctx->cmd)); ret = EINVAL; goto done; }
@@ -1851,8 +1853,8 @@ static int nss_cmd_getbyid(enum sss_cli_command cmd, struct cli_ctx *cctx) } break; default:
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command [%d].\n",
dctx->cmdctx->cmd);
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command [%s].\n",
sss_cli_command_2string(dctx->cmdctx->cmd)); ret = EINVAL; }
@@ -5172,7 +5174,8 @@ static int nss_cmd_getbysid(enum sss_cli_command cmd, struct cli_ctx *cctx) size_t bin_sid_length;
if (cmd != SSS_NSS_GETNAMEBYSID && cmd != SSS_NSS_GETIDBYSID) {
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command type [%d].\n", cmd);
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command type [%s].\n",
sss_cli_command_2string(cmd)); return EINVAL; }
@@ -5214,8 +5217,8 @@ static int nss_cmd_getbysid(enum sss_cli_command cmd, struct cli_ctx *cctx) goto done; }
- DEBUG(SSSDBG_TRACE_FUNC, "Running command [%d] with SID [%s].\n",
dctx->cmdctx->cmd, sid_str);
DEBUG(SSSDBG_TRACE_FUNC, "Running command [%s] with SID [%s].\n",
sss_cli_command_2string(dctx->cmdctx->cmd), sid_str); cmdctx->secid = talloc_strdup(cmdctx, sid_str); if (cmdctx->secid == NULL) {
diff --git a/src/sss_client/pam_sss.c b/src/sss_client/pam_sss.c index e4fa83e12c71bb05dd329686cf2d2df6323ff3bd..90fae56764854d7856b3ee327c18ab6608ff2f6d 100644 --- a/src/sss_client/pam_sss.c +++ b/src/sss_client/pam_sss.c @@ -43,6 +43,7 @@ #include "pam_message.h" #include "util/atomic_io.h" #include "util/authtok-utils.h" +#include "util/util.h"
#include <libintl.h> #define _(STRING) dgettext (PACKAGE, STRING) @@ -1173,7 +1174,7 @@ static int send_and_receive(pam_handle_t *pamh, struct pam_items *pi, case SSS_PAM_PREAUTH: break; default:
D(("Illegal task [%d]", task));
D(("Illegal task [%s]", sss_cli_command_2string(task))); return PAM_SYSTEM_ERR; }
@@ -1580,7 +1581,7 @@ static int pam_sss(enum sss_cli_command task, pam_handle_t *pamh,
bindtextdomain(PACKAGE, LOCALEDIR);
- D(("Hello pam_sssd: %d", task));
D(("Hello pam_sssd: %s", sss_cli_command_2string(task)));
eval_argv(pamh, argc, argv, &flags, &retries, &quiet_mode, &domains);
@@ -1656,7 +1657,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 [%s]", sss_cli_command_2string(task))); return PAM_SYSTEM_ERR; }
diff --git a/src/tools/tools_mc_util.c b/src/tools/tools_mc_util.c index c1b5c616d0e6d50147ecd81308aaa1e69304af92..406c3fad8e2265b0f778ab1f1bf6d25769abb931 100644 --- a/src/tools/tools_mc_util.c +++ b/src/tools/tools_mc_util.c @@ -26,6 +26,7 @@ #include "util/util.h" #include "tools/tools_util.h" #include "util/mmap_cache.h" +#include "util/util.h" #include "sss_client/sss_cli.h"
/* This is a copy of sss_mc_set_recycled present in @@ -226,7 +227,8 @@ static errno_t sss_mc_refresh_ent(const char *name, enum sss_tools_ent ent) }
if (cmd == SSS_CLI_NULL) {
DEBUG(SSSDBG_OP_FAILURE, "Unknown object %d to refresh\n", cmd);
DEBUG(SSSDBG_OP_FAILURE, "Unknown object [%s] to refresh\n",
sss_cli_command_2string(cmd)); return EINVAL; }
diff --git a/src/util/sss_log.c b/src/util/sss_log.c index 48e73dbea99b86c80d9016a4c5d646fc655fcdfb..6e1059395b20e425388c8c3aba39d08ca23e7724 100644 --- a/src/util/sss_log.c +++ b/src/util/sss_log.c @@ -78,6 +78,221 @@ void sss_log_ext(int priority, int facility, const char *format, ...) va_end(ap); }
+const char *sss_cli_command_2string(enum sss_cli_command cmd) {
please put '{' on new line
- switch (cmd) {
- /* null */
- case SSS_CLI_NULL:
return "SSS_CLI_NULL";
- /* version */
- case SSS_GET_VERSION:
return "SSS_GET_VERSION";
- /* passwd */
- case SSS_NSS_GETPWNAM :
return "SSS_NSS_GETPWNAM";
- case SSS_NSS_GETPWUID :
return "SSS_NSS_GETPWUID";
- case SSS_NSS_SETPWENT :
return "SSS_NSS_SETPWENT";
- case SSS_NSS_GETPWENT :
return "SSS_NSS_GETPWENT";
- case SSS_NSS_ENDPWENT :
return "SSS_NSS_ENDPWENT";
- /* group */
- case SSS_NSS_GETGRNAM :
return "SSS_NSS_GETGRNAM";
- case SSS_NSS_GETGRGID :
return "SSS_NSS_GETGRGID";
- case SSS_NSS_SETGRENT :
return "SSS_NSS_SETGRENT";
- case SSS_NSS_GETGRENT :
return "SSS_NSS_GETGRENT";
- case SSS_NSS_ENDGRENT :
return "SSS_NSS_ENDGRENT";
- case SSS_NSS_INITGR :
return "SSS_NSS_INITGR";
- #if 0
- /* aliases */
- case SSS_NSS_GETALIASBYNAME :
return "SSS_NSS_GETALIASBYNAME";
- case SSS_NSS_GETALIASBYPORT :
return "SSS_NSS_GETALIASBYPORT";
- case SSS_NSS_SETALIASENT :
return "SSS_NSS_SETALIASENT";
- case SSS_NSS_GETALIASENT :
return "SSS_NSS_GETALIASENT";
- case SSS_NSS_ENDALIASENT :
return "SSS_NSS_ENDALIASENT";
- /* ethers */
- case SSS_NSS_GETHOSTTON :
return "SSS_NSS_GETHOSTTON";
- case SSS_NSS_GETNTOHOST :
return "SSS_NSS_GETNTOHOST";
- case SSS_NSS_SETETHERENT :
return "SSS_NSS_SETETHERENT";
- case SSS_NSS_GETETHERENT :
return "SSS_NSS_GETETHERENT";
- case SSS_NSS_ENDETHERENT :
return "SSS_NSS_ENDETHERENT";
- /* hosts */
- case SSS_NSS_GETHOSTBYNAME :
return "SSS_NSS_GETHOSTBYNAME";
- case SSS_NSS_GETHOSTBYNAME2 :
return "SSS_NSS_GETHOSTBYNAME2";
- case SSS_NSS_GETHOSTBYADDR :
return "SSS_NSS_GETHOSTBYADDR";
- case SSS_NSS_SETHOSTENT :
return "SSS_NSS_SETHOSTENT";
- case SSS_NSS_GETHOSTENT :
return "SSS_NSS_GETHOSTENT";
- case SSS_NSS_ENDHOSTENT :
return "SSS_NSS_ENDHOSTENT";
- #endif
- /* netgroup */
- case SSS_NSS_SETNETGRENT :
return "SSS_NSS_SETNETGRENT";
- case SSS_NSS_GETNETGRENT :
return "SSS_NSS_GETNETGRENT";
- case SSS_NSS_ENDNETGRENT :
return "SSS_NSS_ENDNETGRENT";
- /* SSS_NSS_INNETGR :
return "SSS_NSS_INNETGR";
break; */
- #if 0
- /* networks */
- case SSS_NSS_GETNETBYNAME :
return "SSS_NSS_GETNETBYNAME";
- case SSS_NSS_GETNETBYADDR :
return "SSS_NSS_GETNETBYADDR";
- case SSS_NSS_SETNETENT :
return "SSS_NSS_SETNETENT";
- case SSS_NSS_GETNETENT :
return "SSS_NSS_GETNETENT";
- case SSS_NSS_ENDNETENT :
return "SSS_NSS_ENDNETENT";
- /* protocols */
- case SSS_NSS_GETPROTOBYNAME :
return "SSS_NSS_GETPROTOBYNAME";
- case SSS_NSS_GETPROTOBYNUM :
return "SSS_NSS_GETPROTOBYNUM";
- case SSS_NSS_SETPROTOENT :
return "SSS_NSS_SETPROTOENT";
- case SSS_NSS_GETPROTOENT :
return "SSS_NSS_GETPROTOENT";
- case SSS_NSS_ENDPROTOENT :
return "SSS_NSS_ENDPROTOENT";
- /* rpc */
- case SSS_NSS_GETRPCBYNAME :
return "SSS_NSS_GETRPCBYNAME";
- case SSS_NSS_GETRPCBYNUM :
return "SSS_NSS_GETRPCBYNUM";
- case SSS_NSS_SETRPCENT :
return "SSS_NSS_SETRPCENT";
- case SSS_NSS_GETRPCENT :
return "SSS_NSS_GETRPCENT";
- case SSS_NSS_ENDRPCENT :
return "SSS_NSS_ENDRPCENT";
- #endif
- /* services */
- case SSS_NSS_GETSERVBYNAME :
return "SSS_NSS_GETSERVBYNAME";
- case SSS_NSS_GETSERVBYPORT :
return "SSS_NSS_GETSERVBYPORT";
- case SSS_NSS_SETSERVENT :
return "SSS_NSS_SETSERVENT";
- case SSS_NSS_GETSERVENT :
return "SSS_NSS_GETSERVENT";
- case SSS_NSS_ENDSERVENT :
return "SSS_NSS_ENDSERVENT";
- #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
- /* SUDO */
- case SSS_SUDO_GET_SUDORULES :
return "SSS_SUDO_GET_SUDORULES";
- case SSS_SUDO_GET_DEFAULTS :
return "SSS_SUDO_GET_DEFAULTS";
- /* autofs */
- case SSS_AUTOFS_SETAUTOMNTENT :
return "SSS_AUTOFS_SETAUTOMNTENT";
- case SSS_AUTOFS_GETAUTOMNTENT :
return "SSS_AUTOFS_GETAUTOMNTENT";
- case SSS_AUTOFS_GETAUTOMNTBYNAME :
return "SSS_AUTOFS_GETAUTOMNTBYNAME";
- case SSS_AUTOFS_ENDAUTOMNTENT :
return "SSS_AUTOFS_ENDAUTOMNTENT";
- /* SSH */
- case SSS_SSH_GET_USER_PUBKEYS :
return "SSS_SSH_GET_USER_PUBKEYS";
- case SSS_SSH_GET_HOST_PUBKEYS :
return "SSS_SSH_GET_HOST_PUBKEYS";
- /* PAM related calls */
- case SSS_PAM_AUTHENTICATE :
return "SSS_PAM_AUTHENTICATE";
- case SSS_PAM_SETCRED :
return "SSS_PAM_SETCRED";
break;
- case SSS_PAM_ACCT_MGMT :
return "SSS_PAM_ACCT_MGMT";
- case SSS_PAM_OPEN_SESSION :
return "SSS_PAM_OPEN_SESSION";
- case SSS_PAM_CLOSE_SESSION :
return "SSS_PAM_CLOSE_SESSION";
- case SSS_PAM_CHAUTHTOK :
return "SSS_PAM_CHAUTHTOK";
- case SSS_PAM_CHAUTHTOK_PRELIM :
return "SSS_PAM_CHAUTHTOK_PRELIM";
- case SSS_CMD_RENEW :
return "SSS_CMD_RENEW";
- case SSS_PAM_PREAUTH :
return "SSS_PAM_PREAUTH";
- /* PAC responder calls */
- case SSS_PAC_ADD_PAC_USER :
return "SSS_PAC_ADD_PAC_USER";
- /* 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 [%d].\n", cmd);
return "UNKNOW COMMAND";
s/UNKNOW/UNKNOWN ?
- }
+}
#ifdef WITH_JOURNALD diff --git a/src/util/util.h b/src/util/util.h index 3d90cf0d1024b93016987a4d3e8a515359fd974d..ce00d8d775555e9c000fd5e491df0b328a00b415 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -47,6 +47,7 @@ #include <dhash.h>
#include "confdb/confdb.h" +#include "sss_client/sss_cli.h"
I think we could avoid adding this include by forward declaring 'enum sss_cli_command' but I don't insist.
#include "util/atomic_io.h" #include "util/util_errors.h" #include "util/util_safealign.h" @@ -239,6 +240,9 @@ void talloc_log_fn(const char *msg); void sss_log(int priority, const char *format, ...) SSS_ATTRIBUTE_PRINTF(2, 3); void sss_log_ext(int priority, int facility, const char *format, ...) SSS_ATTRIBUTE_PRINTF(3, 4);
+/* Translate sss_cli_command to human readable form. */ +const char *sss_cli_command_2string(enum sss_cli_command cmd);
- /* from server.c */ struct main_context { struct tevent_context *event_ctx;
-- 2.4.3
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 07/08/2015 02:46 PM, Pavel Reichl wrote:
On 07/08/2015 02:13 PM, 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.
Petr
Hello Petr,
thank for the patch!
I haven't tested the patch yet, but I have some nitpicks. Please see inline.
0001-UTIL-Function-2string-for-enum-sss_cli_command.patch
From ca782a1518480635ef60bc2cdf77d9b8644132b0 Mon Sep 17 00:00:00 2001 From: Petr Cechpcech@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] with..." We could see:"(0x0400): Running command [SSS_NSS_GETPWNAM] with..."
Is this the right ticket? Subject of the ticket is "Need better libhbac debuging added to sssd" but I don't think this patch relates to libhbac, right?
Pavel, you're right, it is not the right ticket. I was looking to "Improving the debug messages" thread and I would like to start with small improvement. Logging messages could be more on user side. So I will create new ticket for it.
src/responder/nss/nsssrv_cmd.c | 29 +++--- src/sss_client/pam_sss.c | 7 +- src/tools/tools_mc_util.c | 4 +- src/util/sss_log.c | 215 +++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 4 + 5 files changed, 242 insertions(+), 17 deletions(-)
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c index 0129467302f16af318bbbb0a5be47ff2e235da65..421048ec71891b87f6be08efe41fa7c48c97cfaa 100644 --- a/src/responder/nss/nsssrv_cmd.c +++ b/src/responder/nss/nsssrv_cmd.c @@ -1312,7 +1312,8 @@ static int nss_cmd_getbynam(enum sss_cli_command cmd, struct cli_ctx *cctx) case SSS_NSS_GETORIGBYNAME: break; default:
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command type [%d].\n", cmd);
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command type [%s].\n",
sss_cli_command_2string(cmd)); return EINVAL; }
I think that removing the number value might be a little too much. There might be people used to it. So I think that showing both might be the best option. Please see how we use 'sss_strerror' which is similar to your function. Are there other opinions?
Thank you, I'll be inspired.
@@ -1347,8 +1348,8 @@ static int nss_cmd_getbynam(enum sss_cli_command cmd, struct cli_ctx *cctx)
rawname = (const char *)body;
- DEBUG(SSSDBG_TRACE_FUNC, "Running command [%d] with input [%s].\n",
dctx->cmdctx->cmd, rawname);
DEBUG(SSSDBG_TRACE_FUNC, "Running command [%s] with input [%s].\n",
sss_cli_command_2string(dctx->cmdctx->cmd), rawname); if (dctx->cmdctx->cmd == SSS_NSS_GETSIDBYNAME) { ret = nss_check_name_of_well_known_sid(cmdctx, rawname);
@@ -1737,7 +1738,8 @@ static int nss_cmd_getbyid(enum sss_cli_command cmd, struct cli_ctx *cctx) case SSS_NSS_GETSIDBYID: break; default:
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command type [%d].\n", cmd);
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command type [%s].\n",
sss_cli_command_2string(cmd)); return EINVAL; }
@@ -1766,8 +1768,8 @@ static int nss_cmd_getbyid(enum sss_cli_command cmd, struct cli_ctx *cctx) } SAFEALIGN_COPY_UINT32(&cmdctx->id, body, NULL);
- DEBUG(SSSDBG_TRACE_FUNC, "Running command [%d] with id [%"PRIu32"].\n",
dctx->cmdctx->cmd, cmdctx->id);
DEBUG(SSSDBG_TRACE_FUNC, "Running command [%s] with id [%"PRIu32"].\n",
sss_cli_command_2string(dctx->cmdctx->cmd), cmdctx->id); switch(dctx->cmdctx->cmd) { case SSS_NSS_GETPWUID:
@@ -1805,8 +1807,8 @@ static int nss_cmd_getbyid(enum sss_cli_command cmd, struct cli_ctx *cctx) } break; default:
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command [%d].\n",
dctx->cmdctx->cmd);
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command [%s].\n",
sss_cli_command_2string(dctx->cmdctx->cmd)); ret = EINVAL; goto done; }
@@ -1851,8 +1853,8 @@ static int nss_cmd_getbyid(enum sss_cli_command cmd, struct cli_ctx *cctx) } break; default:
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command [%d].\n",
dctx->cmdctx->cmd);
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command [%s].\n",
sss_cli_command_2string(dctx->cmdctx->cmd)); ret = EINVAL; }
@@ -5172,7 +5174,8 @@ static int nss_cmd_getbysid(enum sss_cli_command cmd, struct cli_ctx *cctx) size_t bin_sid_length;
if (cmd != SSS_NSS_GETNAMEBYSID && cmd != SSS_NSS_GETIDBYSID) {
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command type [%d].\n", cmd);
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command type [%s].\n",
sss_cli_command_2string(cmd)); return EINVAL; }
@@ -5214,8 +5217,8 @@ static int nss_cmd_getbysid(enum sss_cli_command cmd, struct cli_ctx *cctx) goto done; }
- DEBUG(SSSDBG_TRACE_FUNC, "Running command [%d] with SID [%s].\n",
dctx->cmdctx->cmd, sid_str);
DEBUG(SSSDBG_TRACE_FUNC, "Running command [%s] with SID [%s].\n",
sss_cli_command_2string(dctx->cmdctx->cmd), sid_str); cmdctx->secid = talloc_strdup(cmdctx, sid_str); if (cmdctx->secid == NULL) {
diff --git a/src/sss_client/pam_sss.c b/src/sss_client/pam_sss.c index e4fa83e12c71bb05dd329686cf2d2df6323ff3bd..90fae56764854d7856b3ee327c18ab6608ff2f6d 100644 --- a/src/sss_client/pam_sss.c +++ b/src/sss_client/pam_sss.c @@ -43,6 +43,7 @@ #include "pam_message.h" #include "util/atomic_io.h" #include "util/authtok-utils.h" +#include "util/util.h"
#include <libintl.h> #define _(STRING) dgettext (PACKAGE, STRING) @@ -1173,7 +1174,7 @@ static int send_and_receive(pam_handle_t *pamh, struct pam_items *pi, case SSS_PAM_PREAUTH: break; default:
D(("Illegal task [%d]", task));
D(("Illegal task [%s]", sss_cli_command_2string(task))); return PAM_SYSTEM_ERR; }
@@ -1580,7 +1581,7 @@ static int pam_sss(enum sss_cli_command task, pam_handle_t *pamh,
bindtextdomain(PACKAGE, LOCALEDIR);
- D(("Hello pam_sssd: %d", task));
D(("Hello pam_sssd: %s", sss_cli_command_2string(task)));
eval_argv(pamh, argc, argv, &flags, &retries, &quiet_mode, &domains);
@@ -1656,7 +1657,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 [%s]", sss_cli_command_2string(task))); return PAM_SYSTEM_ERR; }
diff --git a/src/tools/tools_mc_util.c b/src/tools/tools_mc_util.c index c1b5c616d0e6d50147ecd81308aaa1e69304af92..406c3fad8e2265b0f778ab1f1bf6d25769abb931 100644 --- a/src/tools/tools_mc_util.c +++ b/src/tools/tools_mc_util.c @@ -26,6 +26,7 @@ #include "util/util.h" #include "tools/tools_util.h" #include "util/mmap_cache.h" +#include "util/util.h" #include "sss_client/sss_cli.h"
/* This is a copy of sss_mc_set_recycled present in @@ -226,7 +227,8 @@ static errno_t sss_mc_refresh_ent(const char *name, enum sss_tools_ent ent) }
if (cmd == SSS_CLI_NULL) {
DEBUG(SSSDBG_OP_FAILURE, "Unknown object %d to refresh\n", cmd);
DEBUG(SSSDBG_OP_FAILURE, "Unknown object [%s] to refresh\n",
sss_cli_command_2string(cmd)); return EINVAL; }
diff --git a/src/util/sss_log.c b/src/util/sss_log.c index 48e73dbea99b86c80d9016a4c5d646fc655fcdfb..6e1059395b20e425388c8c3aba39d08ca23e7724 100644 --- a/src/util/sss_log.c +++ b/src/util/sss_log.c @@ -78,6 +78,221 @@ void sss_log_ext(int priority, int facility, const char *format, ...) va_end(ap); }
+const char *sss_cli_command_2string(enum sss_cli_command cmd) {
please put '{' on new line
I'll put, thanks.
- switch (cmd) {
- /* null */
- case SSS_CLI_NULL:
return "SSS_CLI_NULL";
- /* version */
- case SSS_GET_VERSION:
return "SSS_GET_VERSION";
- /* passwd */
- case SSS_NSS_GETPWNAM :
return "SSS_NSS_GETPWNAM";
- case SSS_NSS_GETPWUID :
return "SSS_NSS_GETPWUID";
- case SSS_NSS_SETPWENT :
return "SSS_NSS_SETPWENT";
- case SSS_NSS_GETPWENT :
return "SSS_NSS_GETPWENT";
- case SSS_NSS_ENDPWENT :
return "SSS_NSS_ENDPWENT";
- /* group */
- case SSS_NSS_GETGRNAM :
return "SSS_NSS_GETGRNAM";
- case SSS_NSS_GETGRGID :
return "SSS_NSS_GETGRGID";
- case SSS_NSS_SETGRENT :
return "SSS_NSS_SETGRENT";
- case SSS_NSS_GETGRENT :
return "SSS_NSS_GETGRENT";
- case SSS_NSS_ENDGRENT :
return "SSS_NSS_ENDGRENT";
- case SSS_NSS_INITGR :
return "SSS_NSS_INITGR";
- #if 0
- /* aliases */
- case SSS_NSS_GETALIASBYNAME :
return "SSS_NSS_GETALIASBYNAME";
- case SSS_NSS_GETALIASBYPORT :
return "SSS_NSS_GETALIASBYPORT";
- case SSS_NSS_SETALIASENT :
return "SSS_NSS_SETALIASENT";
- case SSS_NSS_GETALIASENT :
return "SSS_NSS_GETALIASENT";
- case SSS_NSS_ENDALIASENT :
return "SSS_NSS_ENDALIASENT";
- /* ethers */
- case SSS_NSS_GETHOSTTON :
return "SSS_NSS_GETHOSTTON";
- case SSS_NSS_GETNTOHOST :
return "SSS_NSS_GETNTOHOST";
- case SSS_NSS_SETETHERENT :
return "SSS_NSS_SETETHERENT";
- case SSS_NSS_GETETHERENT :
return "SSS_NSS_GETETHERENT";
- case SSS_NSS_ENDETHERENT :
return "SSS_NSS_ENDETHERENT";
- /* hosts */
- case SSS_NSS_GETHOSTBYNAME :
return "SSS_NSS_GETHOSTBYNAME";
- case SSS_NSS_GETHOSTBYNAME2 :
return "SSS_NSS_GETHOSTBYNAME2";
- case SSS_NSS_GETHOSTBYADDR :
return "SSS_NSS_GETHOSTBYADDR";
- case SSS_NSS_SETHOSTENT :
return "SSS_NSS_SETHOSTENT";
- case SSS_NSS_GETHOSTENT :
return "SSS_NSS_GETHOSTENT";
- case SSS_NSS_ENDHOSTENT :
return "SSS_NSS_ENDHOSTENT";
- #endif
- /* netgroup */
- case SSS_NSS_SETNETGRENT :
return "SSS_NSS_SETNETGRENT";
- case SSS_NSS_GETNETGRENT :
return "SSS_NSS_GETNETGRENT";
- case SSS_NSS_ENDNETGRENT :
return "SSS_NSS_ENDNETGRENT";
- /* SSS_NSS_INNETGR :
return "SSS_NSS_INNETGR";
break; */
- #if 0
- /* networks */
- case SSS_NSS_GETNETBYNAME :
return "SSS_NSS_GETNETBYNAME";
- case SSS_NSS_GETNETBYADDR :
return "SSS_NSS_GETNETBYADDR";
- case SSS_NSS_SETNETENT :
return "SSS_NSS_SETNETENT";
- case SSS_NSS_GETNETENT :
return "SSS_NSS_GETNETENT";
- case SSS_NSS_ENDNETENT :
return "SSS_NSS_ENDNETENT";
- /* protocols */
- case SSS_NSS_GETPROTOBYNAME :
return "SSS_NSS_GETPROTOBYNAME";
- case SSS_NSS_GETPROTOBYNUM :
return "SSS_NSS_GETPROTOBYNUM";
- case SSS_NSS_SETPROTOENT :
return "SSS_NSS_SETPROTOENT";
- case SSS_NSS_GETPROTOENT :
return "SSS_NSS_GETPROTOENT";
- case SSS_NSS_ENDPROTOENT :
return "SSS_NSS_ENDPROTOENT";
- /* rpc */
- case SSS_NSS_GETRPCBYNAME :
return "SSS_NSS_GETRPCBYNAME";
- case SSS_NSS_GETRPCBYNUM :
return "SSS_NSS_GETRPCBYNUM";
- case SSS_NSS_SETRPCENT :
return "SSS_NSS_SETRPCENT";
- case SSS_NSS_GETRPCENT :
return "SSS_NSS_GETRPCENT";
- case SSS_NSS_ENDRPCENT :
return "SSS_NSS_ENDRPCENT";
- #endif
- /* services */
- case SSS_NSS_GETSERVBYNAME :
return "SSS_NSS_GETSERVBYNAME";
- case SSS_NSS_GETSERVBYPORT :
return "SSS_NSS_GETSERVBYPORT";
- case SSS_NSS_SETSERVENT :
return "SSS_NSS_SETSERVENT";
- case SSS_NSS_GETSERVENT :
return "SSS_NSS_GETSERVENT";
- case SSS_NSS_ENDSERVENT :
return "SSS_NSS_ENDSERVENT";
- #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
- /* SUDO */
- case SSS_SUDO_GET_SUDORULES :
return "SSS_SUDO_GET_SUDORULES";
- case SSS_SUDO_GET_DEFAULTS :
return "SSS_SUDO_GET_DEFAULTS";
- /* autofs */
- case SSS_AUTOFS_SETAUTOMNTENT :
return "SSS_AUTOFS_SETAUTOMNTENT";
- case SSS_AUTOFS_GETAUTOMNTENT :
return "SSS_AUTOFS_GETAUTOMNTENT";
- case SSS_AUTOFS_GETAUTOMNTBYNAME :
return "SSS_AUTOFS_GETAUTOMNTBYNAME";
- case SSS_AUTOFS_ENDAUTOMNTENT :
return "SSS_AUTOFS_ENDAUTOMNTENT";
- /* SSH */
- case SSS_SSH_GET_USER_PUBKEYS :
return "SSS_SSH_GET_USER_PUBKEYS";
- case SSS_SSH_GET_HOST_PUBKEYS :
return "SSS_SSH_GET_HOST_PUBKEYS";
- /* PAM related calls */
- case SSS_PAM_AUTHENTICATE :
return "SSS_PAM_AUTHENTICATE";
- case SSS_PAM_SETCRED :
return "SSS_PAM_SETCRED";
break;
- case SSS_PAM_ACCT_MGMT :
return "SSS_PAM_ACCT_MGMT";
- case SSS_PAM_OPEN_SESSION :
return "SSS_PAM_OPEN_SESSION";
- case SSS_PAM_CLOSE_SESSION :
return "SSS_PAM_CLOSE_SESSION";
- case SSS_PAM_CHAUTHTOK :
return "SSS_PAM_CHAUTHTOK";
- case SSS_PAM_CHAUTHTOK_PRELIM :
return "SSS_PAM_CHAUTHTOK_PRELIM";
- case SSS_CMD_RENEW :
return "SSS_CMD_RENEW";
- case SSS_PAM_PREAUTH :
return "SSS_PAM_PREAUTH";
- /* PAC responder calls */
- case SSS_PAC_ADD_PAC_USER :
return "SSS_PAC_ADD_PAC_USER";
- /* 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 [%d].\n", cmd);
return "UNKNOW COMMAND";
s/UNKNOW/UNKNOWN ?
- }
+}
#ifdef WITH_JOURNALD diff --git a/src/util/util.h b/src/util/util.h index 3d90cf0d1024b93016987a4d3e8a515359fd974d..ce00d8d775555e9c000fd5e491df0b328a00b415 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -47,6 +47,7 @@ #include <dhash.h>
#include "confdb/confdb.h" +#include "sss_client/sss_cli.h"
I think we could avoid adding this include by forward declaring 'enum sss_cli_command' but I don't insist.
#include "util/atomic_io.h" #include "util/util_errors.h" #include "util/util_safealign.h" @@ -239,6 +240,9 @@ void talloc_log_fn(const char *msg); void sss_log(int priority, const char *format, ...) SSS_ATTRIBUTE_PRINTF(2, 3); void sss_log_ext(int priority, int facility, const char *format, ...) SSS_ATTRIBUTE_PRINTF(3, 4);
+/* Translate sss_cli_command to human readable form. */ +const char *sss_cli_command_2string(enum sss_cli_command cmd);
- /* from server.c */ struct main_context { struct tevent_context *event_ctx;
-- 2.4.3
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
It was only one enum, which is used in logging messages. I supose there could be many of them. What is better idea: a) write "2string" functions near to it's enum declaration, b) create new util module for this?
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hi!
I think the function is a little overkill. The only change I would do in case of sss_cli commands is that I would change the decimal "%d" to a hexa "0x%x" because that matches the representation in the header and would simplify grepping the code.
OTOH if we want to make the messages also readable for the admins it makes sense to use the string representation. But I do not think it is that critical in case of sss_cli commands. But I am not strictly against this approach.
Also you include util.h in the client code. This is something we want to avoid. If the function is really what we want and needs to be used in the client code as well, than it is good to create a small library that will link with the client code as well as the utils.
Michal
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
Hi!
There is my repaired patch. All of yours comments were helpful.
I renamed the function to sss_cmd2str(), but maybe it could be sss_cli_cmd2str(). I am not sure with it, but if it is better, I will rename it again.
Petr
On 07/08/2015 03:26 PM, Sumit Bose wrote:
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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (09/07/15 14:03), Petr Cech wrote:
Hi!
There is my repaired patch. All of yours comments were helpful.
I renamed the function to sss_cmd2str(), but maybe it could be sss_cli_cmd2str(). I am not sure with it, but if it is better, I will rename it again.
Petr
On 07/08/2015 03:26 PM, Sumit Bose wrote:
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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
From 8b3ae05fc97f548256dc8b72863183b9dc9a539a Mon Sep 17 00:00:00 2001 From: Petr Cech pcech@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]..."
Resolves: https://fedorahosted.org/sssd/ticket/2703
Makefile.am | 4 +- 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 | 219 +++++++++++++++++++++++++++++++++++++++ src/util/sss_cli_cmd.h | 9 ++ src/util/sss_log.c | 2 - 8 files changed, 256 insertions(+), 45 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..f16b8ebdb4dd66c2d193c19bd8355782f4de4c9a 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
libsss_debug_la_LIBADD = \ $(SYSLOG_LIBS) libsss_debug_la_LDFLAGS = \ @@ -2654,6 +2655,7 @@ pam_sss_la_SOURCES = \ src/sss_client/sss_cli.h \ src/util/atomic_io.c \ src/util/authtok-utils.c \
- src/util/sss_cli_cmd.c \ src/sss_client/sss_pam_macros.h \ src/sss_client/sss_pam_compat.h
diff --git a/src/providers/dp_pam_data_util.c b/src/providers/dp_pam_data_util.c index 8724bf936f3f46fb8393c8a3da57215a73b4191a..10e91f5f7286db5e76ad98b6c7519f2482d006db 100644 --- a/src/providers/dp_pam_data_util.c +++ b/src/providers/dp_pam_data_util.c @@ -23,33 +23,10 @@ */
#include "providers/data_provider.h"
+#include "util/sss_cli_cmd.h"
#define PAM_SAFE_ITEM(item) item ? item : "not set"
-static const char *pamcmd2str(int cmd) {
- switch (cmd) {
- case SSS_PAM_AUTHENTICATE:
return "PAM_AUTHENTICATE";
- case SSS_PAM_SETCRED:
return "PAM_SETCRED";
- case SSS_PAM_ACCT_MGMT:
return "PAM_ACCT_MGMT";
- case SSS_PAM_OPEN_SESSION:
return "PAM_OPEN_SESSION";
- case SSS_PAM_CLOSE_SESSION:
return "PAM_CLOSE_SESSION";
- case SSS_PAM_CHAUTHTOK:
return "PAM_CHAUTHTOK";
- case SSS_PAM_CHAUTHTOK_PRELIM:
return "PAM_CHAUTHTOK_PRELIM";
- case SSS_PAM_PREAUTH:
return "SSS_PAM_PREAUTH";
- default:
return "UNKNOWN";
- }
-}
int pam_data_destructor(void *ptr) { struct pam_data *pd = talloc_get_type(ptr, struct pam_data); @@ -183,7 +160,7 @@ failed:
void pam_print_data(int l, struct pam_data *pd) {
- DEBUG(l, "command: %s\n", pamcmd2str(pd->cmd));
- DEBUG(l, "command: %s\n", sss_cmd2str(pd->cmd)); DEBUG(l, "domain: %s\n", PAM_SAFE_ITEM(pd->domain)); DEBUG(l, "user: %s\n", PAM_SAFE_ITEM(pd->user)); DEBUG(l, "service: %s\n", PAM_SAFE_ITEM(pd->service));
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 @@ -21,6 +21,7 @@
#include "util/util.h" #include "util/sss_nss.h" +#include "util/sss_cli_cmd.h" #include "responder/nss/nsssrv.h" #include "responder/nss/nsssrv_private.h" #include "responder/nss/nsssrv_netgroup.h" @@ -1312,7 +1313,8 @@ static int nss_cmd_getbynam(enum sss_cli_command cmd, struct cli_ctx *cctx) case SSS_NSS_GETORIGBYNAME: break; default:
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command type [%d].\n", cmd);
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command type [%d][%s].\n",
}cmd, sss_cmd2str(cmd)); return EINVAL;
@@ -1347,8 +1349,8 @@ static int nss_cmd_getbynam(enum sss_cli_command cmd, struct cli_ctx *cctx)
rawname = (const char *)body;
- DEBUG(SSSDBG_TRACE_FUNC, "Running command [%d] with input [%s].\n",
dctx->cmdctx->cmd, rawname);
DEBUG(SSSDBG_TRACE_FUNC, "Running command [%d][%s] with input [%s].\n",
cmd, sss_cmd2str(dctx->cmdctx->cmd), rawname);
if (dctx->cmdctx->cmd == SSS_NSS_GETSIDBYNAME) { ret = nss_check_name_of_well_known_sid(cmdctx, rawname);
@@ -1737,7 +1739,8 @@ static int nss_cmd_getbyid(enum sss_cli_command cmd, struct cli_ctx *cctx) case SSS_NSS_GETSIDBYID: break; default:
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command type [%d].\n", cmd);
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command type [%d][%s].\n",
}cmd, sss_cmd2str(cmd)); return EINVAL;
@@ -1766,8 +1769,8 @@ static int nss_cmd_getbyid(enum sss_cli_command cmd, struct cli_ctx *cctx) } SAFEALIGN_COPY_UINT32(&cmdctx->id, body, NULL);
- DEBUG(SSSDBG_TRACE_FUNC, "Running command [%d] with id [%"PRIu32"].\n",
dctx->cmdctx->cmd, cmdctx->id);
DEBUG(SSSDBG_TRACE_FUNC, "Running command [%d][%s] with id [%"PRIu32"].\n",
cmd, sss_cmd2str(dctx->cmdctx->cmd), cmdctx->id);
switch(dctx->cmdctx->cmd) { case SSS_NSS_GETPWUID:
@@ -1805,8 +1808,8 @@ static int nss_cmd_getbyid(enum sss_cli_command cmd, struct cli_ctx *cctx) } break; default:
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command [%d].\n",
dctx->cmdctx->cmd);
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command [%d][%s].\n",
}cmd, sss_cmd2str(dctx->cmdctx->cmd)); ret = EINVAL; goto done;
@@ -1851,8 +1854,8 @@ static int nss_cmd_getbyid(enum sss_cli_command cmd, struct cli_ctx *cctx) } break; default:
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command [%d].\n",
dctx->cmdctx->cmd);
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command [%d][%s].\n",
}cmd, sss_cmd2str(dctx->cmdctx->cmd)); ret = EINVAL;
@@ -5172,7 +5175,8 @@ static int nss_cmd_getbysid(enum sss_cli_command cmd, struct cli_ctx *cctx) size_t bin_sid_length;
if (cmd != SSS_NSS_GETNAMEBYSID && cmd != SSS_NSS_GETIDBYSID) {
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command type [%d].\n", cmd);
DEBUG(SSSDBG_CRIT_FAILURE, "Invalid command type [%d][%s].\n",
}cmd, sss_cmd2str(cmd)); return EINVAL;
@@ -5214,8 +5218,8 @@ static int nss_cmd_getbysid(enum sss_cli_command cmd, struct cli_ctx *cctx) goto done; }
- DEBUG(SSSDBG_TRACE_FUNC, "Running command [%d] with SID [%s].\n",
dctx->cmdctx->cmd, sid_str);
DEBUG(SSSDBG_TRACE_FUNC, "Running command [%d][%s] with SID [%s].\n",
cmd, sss_cmd2str(dctx->cmdctx->cmd), sid_str);
cmdctx->secid = talloc_strdup(cmdctx, sid_str); if (cmdctx->secid == NULL) {
diff --git a/src/sss_client/pam_sss.c b/src/sss_client/pam_sss.c index e4fa83e12c71bb05dd329686cf2d2df6323ff3bd..541b9240edc7616cc28d291c6117227e5a1e9133 100644 --- a/src/sss_client/pam_sss.c +++ b/src/sss_client/pam_sss.c @@ -1173,7 +1173,7 @@ static int send_and_receive(pam_handle_t *pamh, struct pam_items *pi, case SSS_PAM_PREAUTH: break; default:
D(("Illegal task [%d]", task));
}D(("Illegal task [%d][%s]", task, sss_cmd2str(task))); return PAM_SYSTEM_ERR;
@@ -1580,7 +1580,7 @@ static int pam_sss(enum sss_cli_command task, pam_handle_t *pamh,
bindtextdomain(PACKAGE, LOCALEDIR);
- D(("Hello pam_sssd: %d", task));
D(("Hello pam_sssd: %d %s", task, sss_cmd2str(task)));
eval_argv(pamh, argc, argv, &flags, &retries, &quiet_mode, &domains);
@@ -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 [%d][%s]",task, sss_cmd2str(task))); return PAM_SYSTEM_ERR; }
I replied to the different mail in this thread that I think we needn't use string representation in client code. Hexadecimal should be sufficient.
But let's wait for and agreement.
diff --git a/src/tools/tools_mc_util.c b/src/tools/tools_mc_util.c index c1b5c616d0e6d50147ecd81308aaa1e69304af92..38f9d42ce6a0759efa7e10932d0b655d6d366461 100644 --- a/src/tools/tools_mc_util.c +++ b/src/tools/tools_mc_util.c @@ -26,6 +26,7 @@ #include "util/util.h" #include "tools/tools_util.h" #include "util/mmap_cache.h" +#include "util/sss_cli_cmd.h" #include "sss_client/sss_cli.h"
/* This is a copy of sss_mc_set_recycled present in @@ -226,7 +227,8 @@ static errno_t sss_mc_refresh_ent(const char *name, enum sss_tools_ent ent) }
if (cmd == SSS_CLI_NULL) {
DEBUG(SSSDBG_OP_FAILURE, "Unknown object %d to refresh\n", cmd);
DEBUG(SSSDBG_OP_FAILURE, "Unknown object [%d][%s] to refresh\n",
}cmd, sss_cmd2str(cmd)); return EINVAL;
diff --git a/src/util/sss_cli_cmd.c b/src/util/sss_cli_cmd.c new file mode 100644 index 0000000000000000000000000000000000000000..e160bb700744d7b56ee0a02c13cccffc3529f5dc --- /dev/null +++ b/src/util/sss_cli_cmd.c @@ -0,0 +1,219 @@ +#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) +{
- switch (cmd) {
- /* null */
- case SSS_CLI_NULL:
return "SSS_CLI_NULL";
- /* version */
- case SSS_GET_VERSION:
return "SSS_GET_VERSION";
- /* passwd */
- case SSS_NSS_GETPWNAM:
return "SSS_NSS_GETPWNAM";
- case SSS_NSS_GETPWUID:
return "SSS_NSS_GETPWUID";
- case SSS_NSS_SETPWENT:
return "SSS_NSS_SETPWENT";
- case SSS_NSS_GETPWENT:
return "SSS_NSS_GETPWENT";
- case SSS_NSS_ENDPWENT:
return "SSS_NSS_ENDPWENT";
- /* group */
- case SSS_NSS_GETGRNAM:
return "SSS_NSS_GETGRNAM";
- case SSS_NSS_GETGRGID:
return "SSS_NSS_GETGRGID";
- case SSS_NSS_SETGRENT:
return "SSS_NSS_SETGRENT";
- case SSS_NSS_GETGRENT:
return "SSS_NSS_GETGRENT";
- case SSS_NSS_ENDGRENT:
return "SSS_NSS_ENDGRENT";
- case SSS_NSS_INITGR:
return "SSS_NSS_INITGR";
- #if 0
- /* aliases */
- case SSS_NSS_GETALIASBYNAME:
return "SSS_NSS_GETALIASBYNAME";
- case SSS_NSS_GETALIASBYPORT:
return "SSS_NSS_GETALIASBYPORT";
- case SSS_NSS_SETALIASENT:
return "SSS_NSS_SETALIASENT";
- case SSS_NSS_GETALIASENT:
return "SSS_NSS_GETALIASENT";
- case SSS_NSS_ENDALIASENT:
return "SSS_NSS_ENDALIASENT";
- /* ethers */
- case SSS_NSS_GETHOSTTON:
return "SSS_NSS_GETHOSTTON";
- case SSS_NSS_GETNTOHOST:
return "SSS_NSS_GETNTOHOST";
- case SSS_NSS_SETETHERENT:
return "SSS_NSS_SETETHERENT";
- case SSS_NSS_GETETHERENT:
return "SSS_NSS_GETETHERENT";
- case SSS_NSS_ENDETHERENT:
return "SSS_NSS_ENDETHERENT";
- /* hosts */
- case SSS_NSS_GETHOSTBYNAME:
return "SSS_NSS_GETHOSTBYNAME";
- case SSS_NSS_GETHOSTBYNAME2:
return "SSS_NSS_GETHOSTBYNAME2";
- case SSS_NSS_GETHOSTBYADDR:
return "SSS_NSS_GETHOSTBYADDR";
- case SSS_NSS_SETHOSTENT:
return "SSS_NSS_SETHOSTENT";
- case SSS_NSS_GETHOSTENT:
return "SSS_NSS_GETHOSTENT";
- case SSS_NSS_ENDHOSTENT:
return "SSS_NSS_ENDHOSTENT";
- #endif
- /* netgroup */
- case SSS_NSS_SETNETGRENT:
return "SSS_NSS_SETNETGRENT";
- case SSS_NSS_GETNETGRENT:
return "SSS_NSS_GETNETGRENT";
- case SSS_NSS_ENDNETGRENT:
return "SSS_NSS_ENDNETGRENT";
- /* SSS_NSS_INNETGR:
return "SSS_NSS_INNETGR";
break; */
- #if 0
- /* networks */
- case SSS_NSS_GETNETBYNAME:
return "SSS_NSS_GETNETBYNAME";
- case SSS_NSS_GETNETBYADDR:
return "SSS_NSS_GETNETBYADDR";
- case SSS_NSS_SETNETENT:
return "SSS_NSS_SETNETENT";
- case SSS_NSS_GETNETENT:
return "SSS_NSS_GETNETENT";
- case SSS_NSS_ENDNETENT:
return "SSS_NSS_ENDNETENT";
- /* protocols */
- case SSS_NSS_GETPROTOBYNAME:
return "SSS_NSS_GETPROTOBYNAME";
- case SSS_NSS_GETPROTOBYNUM:
return "SSS_NSS_GETPROTOBYNUM";
- case SSS_NSS_SETPROTOENT:
return "SSS_NSS_SETPROTOENT";
- case SSS_NSS_GETPROTOENT:
return "SSS_NSS_GETPROTOENT";
- case SSS_NSS_ENDPROTOENT:
return "SSS_NSS_ENDPROTOENT";
- /* rpc */
- case SSS_NSS_GETRPCBYNAME:
return "SSS_NSS_GETRPCBYNAME";
- case SSS_NSS_GETRPCBYNUM:
return "SSS_NSS_GETRPCBYNUM";
- case SSS_NSS_SETRPCENT:
return "SSS_NSS_SETRPCENT";
- case SSS_NSS_GETRPCENT:
return "SSS_NSS_GETRPCENT";
- case SSS_NSS_ENDRPCENT:
return "SSS_NSS_ENDRPCENT";
- #endif
- /* services */
- case SSS_NSS_GETSERVBYNAME:
return "SSS_NSS_GETSERVBYNAME";
- case SSS_NSS_GETSERVBYPORT:
return "SSS_NSS_GETSERVBYPORT";
- case SSS_NSS_SETSERVENT:
return "SSS_NSS_SETSERVENT";
- case SSS_NSS_GETSERVENT:
return "SSS_NSS_GETSERVENT";
- case SSS_NSS_ENDSERVENT:
return "SSS_NSS_ENDSERVENT";
- #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
- /* SUDO */
- case SSS_SUDO_GET_SUDORULES:
return "SSS_SUDO_GET_SUDORULES";
- case SSS_SUDO_GET_DEFAULTS:
return "SSS_SUDO_GET_DEFAULTS";
- /* autofs */
- case SSS_AUTOFS_SETAUTOMNTENT:
return "SSS_AUTOFS_SETAUTOMNTENT";
- case SSS_AUTOFS_GETAUTOMNTENT:
return "SSS_AUTOFS_GETAUTOMNTENT";
- case SSS_AUTOFS_GETAUTOMNTBYNAME:
return "SSS_AUTOFS_GETAUTOMNTBYNAME";
- case SSS_AUTOFS_ENDAUTOMNTENT:
return "SSS_AUTOFS_ENDAUTOMNTENT";
- /* SSH */
- case SSS_SSH_GET_USER_PUBKEYS:
return "SSS_SSH_GET_USER_PUBKEYS";
- case SSS_SSH_GET_HOST_PUBKEYS:
return "SSS_SSH_GET_HOST_PUBKEYS";
- /* PAM related calls */
- case SSS_PAM_AUTHENTICATE:
return "SSS_PAM_AUTHENTICATE";
- case SSS_PAM_SETCRED:
return "SSS_PAM_SETCRED";
- case SSS_PAM_ACCT_MGMT:
return "SSS_PAM_ACCT_MGMT";
- case SSS_PAM_OPEN_SESSION:
return "SSS_PAM_OPEN_SESSION";
- case SSS_PAM_CLOSE_SESSION:
return "SSS_PAM_CLOSE_SESSION";
- case SSS_PAM_CHAUTHTOK:
return "SSS_PAM_CHAUTHTOK";
- case SSS_PAM_CHAUTHTOK_PRELIM:
return "SSS_PAM_CHAUTHTOK_PRELIM";
- case SSS_CMD_RENEW:
return "SSS_CMD_RENEW";
- case SSS_PAM_PREAUTH:
return "SSS_PAM_PREAUTH";
- /* PAC responder calls */
- case SSS_PAC_ADD_PAC_USER:
return "SSS_PAC_ADD_PAC_USER";
- /* 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 [%d].\n", cmd);
return "UNKNOWN COMMAND";
- }
+} diff --git a/src/util/sss_cli_cmd.h b/src/util/sss_cli_cmd.h new file mode 100644 index 0000000000000000000000000000000000000000..4bd18af85fcca0bcfbcc28ddbcc99bfaaac2c7c4 --- /dev/null +++ b/src/util/sss_cli_cmd.h @@ -0,0 +1,9 @@
There is missing header with licence. The same applies to the new implemetatation module.
+#ifndef __SSS_CLI_CMD_H__ +#define __SSS_CLI_CMD_H__
+#include "sss_client/sss_cli.h"
+/* Translate sss_cli_command to human readable form. */ +const char *sss_cmd2str(enum sss_cli_command cmd);
+#endif /* __SSS_CLI_CMD_H__ */
diff --git a/src/util/sss_log.c b/src/util/sss_log.c index 48e73dbea99b86c80d9016a4c5d646fc655fcdfb..250c1d6dc93569250e552b9e4cd2f2e3ebcc0745 100644 --- a/src/util/sss_log.c +++ b/src/util/sss_log.c @@ -78,8 +78,6 @@ void sss_log_ext(int priority, int facility, const char *format, ...) va_end(ap); }
This change is unrelated to the ticket. It's fine to change it if you touch code nearby.
#ifdef WITH_JOURNALD
static void sss_log_internal(int priority, int facility, const char *format,
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
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.
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.
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)
LS
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.
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
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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hi,
there is another repaired patch. Changes are: * hexadecimal numbers instead of cmd2str() in sss_client, * added license preamble in headers of new files.
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
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@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.)
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@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
On 08/13/2015 11:11 AM, Lukas Slebodnik wrote:
From a93e36f11759cf9a748942e7632d4a07a088b098 Mon Sep 17 00:00:00 2001 From: Petr Cech pcech@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.)
The patch does not apply to master. I had to use tree way merge. Please rebase it.
Rebased.
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.
$NULL added.
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.
Returned.
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@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.
Removed.
- /* 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 :-)
One space added :-)
Otherwise LGTM
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
And there is new patched attached. There were some another patches, so I append new logs by sss_cmd2str() too.
Petr
On (18/08/15 15:54), Petr Cech wrote:
On 08/13/2015 11:11 AM, Lukas Slebodnik wrote:
From a93e36f11759cf9a748942e7632d4a07a088b098 Mon Sep 17 00:00:00 2001 From: Petr Cech pcech@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.)
The patch does not apply to master. I had to use tree way merge. Please rebase it.
Rebased.
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.
$NULL added.
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.
Returned.
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@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.
Removed.
- /* 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 :-)
One space added :-)
Otherwise LGTM
LS
And there is new patched attached. There were some another patches, so I append new logs by sss_cmd2str() too.
Petr
From d561a20c6db3aca21424281c49808f4992332ee2 Mon Sep 17 00:00:00 2001 From: Petr Cech pcech@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
ACK
LS
On Mon, Aug 31, 2015 at 06:33:52PM +0200, Jakub Hrozek wrote:
On Thu, Aug 27, 2015 at 12:19:18PM +0200, Lukas Slebodnik wrote:
ACK
LS
- master: 11e8f3ecdddf8edd8b1bbe9f41b49ce8b709b92a
This patch broke distcheck:
../src/util/sss_cli_cmd.c -fPIC -DPIC -o src/util/.libs/sss_cli_cmd.o ../src/util/sss_cli_cmd.c:21:30: fatal error: util/sss_cli_cmd.h: No such file or directory #include "util/sss_cli_cmd.h" ^
Please fix ASAP..
On 08/31/2015 09:51 PM, Jakub Hrozek wrote:
On Mon, Aug 31, 2015 at 06:33:52PM +0200, Jakub Hrozek wrote:
On Thu, Aug 27, 2015 at 12:19:18PM +0200, Lukas Slebodnik wrote:
ACK
LS
- master: 11e8f3ecdddf8edd8b1bbe9f41b49ce8b709b92a
This patch broke distcheck:
../src/util/sss_cli_cmd.c -fPIC -DPIC -o src/util/.libs/sss_cli_cmd.o ../src/util/sss_cli_cmd.c:21:30: fatal error: util/sss_cli_cmd.h: No such file or directory #include "util/sss_cli_cmd.h" ^
Please fix ASAP.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I am sorry. There is a fix attached.
Petr
On 09/01/2015 08:44 AM, Petr Cech wrote:
On 08/31/2015 09:51 PM, Jakub Hrozek wrote:
On Mon, Aug 31, 2015 at 06:33:52PM +0200, Jakub Hrozek wrote:
On Thu, Aug 27, 2015 at 12:19:18PM +0200, Lukas Slebodnik wrote:
ACK
LS
- master: 11e8f3ecdddf8edd8b1bbe9f41b49ce8b709b92a
This patch broke distcheck:
../src/util/sss_cli_cmd.c -fPIC -DPIC -o src/util/.libs/sss_cli_cmd.o ../src/util/sss_cli_cmd.c:21:30: fatal error: util/sss_cli_cmd.h: No such file or directory #include "util/sss_cli_cmd.h" ^
Please fix ASAP.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I am sorry. There is a fix attached.
Petr
Patch fixed the problem for me. ACK
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Tue, Sep 01, 2015 at 09:37:29AM +0200, Pavel Reichl wrote:
On 09/01/2015 08:44 AM, Petr Cech wrote:
On 08/31/2015 09:51 PM, Jakub Hrozek wrote:
On Mon, Aug 31, 2015 at 06:33:52PM +0200, Jakub Hrozek wrote:
On Thu, Aug 27, 2015 at 12:19:18PM +0200, Lukas Slebodnik wrote:
ACK
LS
- master: 11e8f3ecdddf8edd8b1bbe9f41b49ce8b709b92a
This patch broke distcheck:
../src/util/sss_cli_cmd.c -fPIC -DPIC -o src/util/.libs/sss_cli_cmd.o ../src/util/sss_cli_cmd.c:21:30: fatal error: util/sss_cli_cmd.h: No such file or directory #include "util/sss_cli_cmd.h" ^
Please fix ASAP.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I am sorry. There is a fix attached.
Petr
Patch fixed the problem for me. ACK
* master: 46e36286953de4e5af5e4289b90a529929bdd17c
sssd-devel@lists.fedorahosted.org