On (04/11/14 18:09), Sumit Bose wrote:
On Tue, Nov 04, 2014 at 12:17:43PM +0100, Jakub Hrozek wrote:
> On Wed, Oct 29, 2014 at 01:01:43PM +0100, Sumit Bose wrote:
> > I agree, I allowed user_attributes for the nss responder as well with a
> > fallback to the InfoPipe value so the admin gets it for free if InfoPipe
> > is already configured but does not have to configure InfoPipe if not
> > needed. See man page change for details.
> >
> > Thank you for the review, new version attached.
> >
> > bye,
> > Sumit
>
> I'm sorry about the delay in review.
>
> This version looks almost good, but I have one more request -- can we
> move ifp_parse_attr_list_ex into responder_common.c and drop the ifp
> prefix?
I kind of expected this :-) I was a bit reluctant because I was afraid
to break something in InfoPipe. I created a new file responder_utils.c
for this because responder_common.c looks already quite big.
New version attached.
bye,
Sumit
>
> What that would prevent in particular is:
> > diff --git a/src/responder/nss/nsssrv.c b/src/responder/nss/nsssrv.c
> > index 1bbeaa1..601ffc2 100644
> > --- a/src/responder/nss/nsssrv.c
> > +++ b/src/responder/nss/nsssrv.c
> > @@ -48,6 +48,7 @@
> > #include "monitor/monitor_interfaces.h"
> > #include "sbus/sbus_client.h"
> > #include "util/util_sss_idmap.h"
> > +#include "responder/ifp/ifp_private.h"
> >
>
> I know we're currently only using a single function, but it's not very
> nice to use a private interface of another responder.
>
> I'm fine with the rest of the code.
From 52842484474caa2e91ab39c2ef7edc538175aced Mon Sep 17 00:00:00
2001
From: Sumit Bose <sbose(a)redhat.com>
Date: Tue, 28 Oct 2014 19:42:47 +0100
Subject: [PATCH 3/4] nss: parse user_attributes option
---
src/man/sssd.conf.5.xml | 26 ++++++++++++++++++++++++++
src/responder/nss/nsssrv.c | 17 +++++++++++++++++
src/responder/nss/nsssrv.h | 2 ++
3 files changed, 45 insertions(+)
//snip
+++ b/src/responder/nss/nsssrv.c
@@ -214,6 +214,7 @@ static int nss_get_config(struct nss_ctx *nctx,
struct confdb_ctx *cdb)
{
int ret;
+ char *tmp_str;
ret = confdb_get_int(cdb, CONFDB_NSS_CONF_ENTRY,
CONFDB_NSS_ENUM_CACHE_TIMEOUT, 120,
@@ -298,6 +299,22 @@ static int nss_get_config(struct nss_ctx *nctx,
&nctx->homedir_substr);
if (ret != EOK) goto done;
+
+ ret = confdb_get_string(cdb, nctx, CONFDB_NSS_CONF_ENTRY,
+ CONFDB_IFP_USER_ATTR_LIST, NULL, &tmp_str);
+ if (ret != EOK) goto done;
+
+ if (tmp_str == NULL) {
+ ret = confdb_get_string(cdb, nctx, CONFDB_IFP_CONF_ENTRY,
+ CONFDB_IFP_USER_ATTR_LIST, NULL, &tmp_str);
+ if (ret != EOK) goto done;
+ }
+
+ if (tmp_str != NULL) {
+ nctx->extra_attributes = parse_attr_list_ex(nctx, tmp_str, NULL);
+ if (ret != EOK) goto done;
^^^^^^^^^^
It would be better to test "nctx->extra_attributes" rather than
ret
LS