[PATCH] Refactoring: remove duplicated code in nss responder

[Sumit Bose]

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

[Jakub Hrozek]

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&gt;
>  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.

[Sumit Bose]

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&gt;
>  > 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
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

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&gt;
>  > > 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

[Jakub Hrozek]

On Sun, 2013-04-21 at 11:21 +0200, Jakub Hrozek wrote:
>  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&gt;
>  > > > 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
>   
Pushed to master