[SSSD] [PATCH] Refactoring: remove duplicated code in nss responder

Jakub Hrozek jhrozek at redhat.com
Fri Apr 19 17:47:09 UTC 2013


On Mon, Apr 15, 2013 at 10:34:07PM +0200, Sumit Bose wrote:
> Hi,
> 
> while working on adding new nss responder calls for SID related lookups
> I realized that I can copy-and-paste some existing code again and add
> even more duplication or start to refactor some of the existing code in
> the nss responder. It preferred to second and the attached pathc is the
> result. There is still lots of duplicated code in the nss responder, but
> this patch would at least help to add the SID lookups without adding
> redundancy.
> 
> bye,
> Sumit

> From 2fa083ceaf72360bbacfbe576d0e9378bd7b3444 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Mon, 15 Apr 2013 10:58:05 +0200
> Subject: [PATCH] Refactoring: remove duplicated code in nss responder
> 
> Different user and group lookup requests used nearly identical code,
> this patch unifies some of the related code paths.
> ---
>  src/responder/nss/nsssrv_cmd.c     |  861 ++++++++++--------------------------
>  src/responder/nss/nsssrv_private.h |    1 +
>  2 files changed, 238 insertions(+), 624 deletions(-)
I really like this  ^^^^^^^^^^^^^^^^^^^^^^

The patch seems to work fine, I tested all the usual operations and the
code looks good to me. I would just like to ask for some nitpicks to be
changed:

> +static void nss_cmd_getby_dp_callback(uint16_t err_maj, uint32_t err_min,
> +                                         const char *err_msg, void *ptr);

                                         ^^^
                                      indentation

> @@ -883,11 +955,18 @@ static int nss_cmd_getpwnam(struct cli_ctx *cctx)
>      size_t blen;
>      int ret;
>  
> +    if (cmd != SSS_NSS_GETPWNAM && cmd != SSS_NSS_GETGRNAM &&
> +        cmd != SSS_NSS_INITGR) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, ("Invalid command type [%d].\n", cmd));
> +        return EINVAL;
> +    }
> +

Would a switch/case be more readable here?

switch(cmd) {
case SSS_NSS_GETPWNAM:
case SSS_NSS_GETGRNAM:
case SSS_NSS_INITGR:
    break;
default:
    DEBUG(SSSDBG_CRIT_FAILURE, ("Invalid command type [%d].\n", cmd));
    return EINVAL;
}

> @@ -923,11 +1005,11 @@ static int nss_cmd_getpwnam(struct cli_ctx *cctx)
>              ret = ENOMEM;
>          } else {
>              dctx->rawname = rawname;
> -            tevent_req_set_callback(req, nss_cmd_getpwnam_cb, dctx);
> +            tevent_req_set_callback(req, nss_cmd_getbynam_cb, dctx);

I know it was the case also before but can you change the name of the
callback to follow the tevent_req style? Something like
nss_cmd_getbynam_dom_done ?

Otherwise ack.



More information about the sssd-devel mailing list