On 07/08/2015 02:13 PM, Petr Cech wrote: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.
Hello Petr,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
thank for the patch!
I haven't tested the patch yet, but I have some nitpicks. Please see inline.
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?
From ca782a1518480635ef60bc2cdf77d9b8644132b0 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] with..." We could see:"(0x0400): Running command [SSS_NSS_GETPWNAM] with..." Resolves: https://fedorahosted.org/sssd/ticket/2703
Thank you, I'll be inspired.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.--- 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; }
Are there other opinions?
I'll put, thanks.please put '{' on new line@@ -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) {
s/UNKNOW/UNKNOWN ?+ 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";
It was only one enum, which is used in logging messages. I supose there could be many of them. What is better idea:I think we could avoid adding this include by forward declaring 'enum sss_cli_command' but I don't insist.+ } +} #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"
#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
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel