On Fri, 2013-04-19 at 21:48 +0200, Sumit Bose wrote:
On Fri, Apr 19, 2013 at 07:47:09PM +0200, Jakub Hrozek wrote:
> 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(a)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.
Thank you for the review, I did all the changes you suggested. New
version attached.
bye,
Sumit
Ack