On Tue, Nov 04, 2014 at 07:02:34PM +0100, Lukas Slebodnik wrote:
On (04/11/14 18:41), Lukas Slebodnik wrote:
>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
>
I sent mail very early :-)
These patches depends on another patch set.
CC src/responder/nss/nsssrv_cmd.o
src/responder/nss/nsssrv_cmd.c: In function ‘nss_cmd_getsidby_search’:
src/responder/nss/nsssrv_cmd.c:4259:13: error: implicit declaration of function
‘add_strings_lists’ [-Werror=implicit-function-declaration]
ret = add_strings_lists(cmdctx, default_attrs,
^
cc1: all warnings being treated as errors
LS
Yes, we talked about that with Sumit on IRC. I'll reorder the patches
before pushing.