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 <sb...@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.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
