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

Petr Cech pcech at redhat.com
Wed Jul 8 13:34:58 UTC 2015



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 Cech<pcech at redhat.com>
>> Date: Wed, 8 Jul 2015 07:17:28 -0400
>> Subject: [PATCH] UTIL: Function 2string for enum sss_cli_command
>>
>> Improvement of debug messages.
>> Instead of:"(0x0400): Running command [17] with..."
>> We could see:"(0x0400): Running command [SSS_NSS_GETPWNAM] with..."
>>
>> Resolves:
>> https://fedorahosted.org/sssd/ticket/2703
> 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 at 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 at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150708/4c801a39/attachment.html>


More information about the sssd-devel mailing list