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

Jakub Hrozek jakub.hrozek at gmail.com
Sun Apr 21 09:21:56 UTC 2013


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 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.
> 
> Thank you for the review, I did all the changes you suggested. New
> version attached.
> 
> bye,
> Sumit

Ack




More information about the sssd-devel mailing list