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 <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.
>
> Thank you for the review, I did all the changes you suggested. New
> version attached.
>
> bye,
> Sumit
Ack
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
