On Wed, Nov 05, 2014 at 11:28:02AM +0100, Jakub Hrozek wrote:
> On Tue, Nov 04, 2014 at 08:49:19PM +0100, Sumit Bose wrote:
> > On Tue, Nov 04, 2014 at 07:15:05PM +0100, Jakub Hrozek wrote:
> > > On Tue, Nov 04, 2014 at 07:07:11PM +0100, Jakub Hrozek wrote:
> > > > 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
> >
> > d'oh, fixed
> >
> > > > > >
> > > > >
> > > > > 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.
> > >
> > > btw I don't have any more comments, I can even fix the check before
> > > pushing..
> > >
> > > The code works fine:
> > > $ dbus-send --print-reply --system --dest=org.freedesktop.sssd.infopipe
> > > /org/freedesktop/sssd/infopipe
org.freedesktop.sssd.infopipe.GetUserAttr
> > > string:psuser@ad.example.com array:string:name,gecos,phone
> > > method return sender=:1.27 -> dest=:1.29 reply_serial=2
> > > array [
> > > dict entry(
> > > string "name"
> > > variant array [
> > > string "psuser(a)AD.EXAMPLE.COM"
> > > ]
> > > )
> > > dict entry(
> > > string "gecos"
> > > variant array [
> > > string "ps user"
> > > ]
> > > )
> > > dict entry(
> > > string "phone"
> > > variant array [
> > > string "444-333"
> > > ]
> > > )
> > > ]
> > >
> > > I had to add phone to the allowed attrs to the nss section on the IPA
> > > server itself (now I see the convenience of using ldap_extra_attrs right
> > > away..) and to the ifp section on the client.
> >
> > Thank you Lukas and Jakub.
> >
> > I added the add_strings_lists() patch to this set to get the order
> > right.
> >
> > bye,
> > Sumit
> > > _______________________________________________
> > > sssd-devel mailing list
> > > sssd-devel(a)lists.fedorahosted.org
> > >
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> If you agree, I will squash in the following change:
>
> diff --git a/src/responder/nss/nsssrv.c b/src/responder/nss/nsssrv.c
> index baea9d5..bce06c3 100644
> --- a/src/responder/nss/nsssrv.c
> +++ b/src/responder/nss/nsssrv.c
> @@ -312,7 +312,10 @@ static int nss_get_config(struct nss_ctx *nctx,
>
> if (tmp_str != NULL) {
> nctx->extra_attributes = parse_attr_list_ex(nctx, tmp_str, NULL);
> - if (ret != EOK) goto done;
> + if (nctx->extra_attributes == NULL) {
> + ret = ENOMEM;
> + goto done;
> + }
> }
>
> ret = 0;
yes, I have the same patch in my tree but failed to add it to the latest
set.
bye,
Sumit
Pushed to master:
* e4549c5364461644723361d688badde7fe137a25
* 166ddd0dfbda28b1c6773f386bb7ff88914af91a
* 115de6d50f0d0bdd5745a5d8eb0d067be9128528
* 1f7844eb0aa4b19247533aa83f1cb4876396c738
* 9ce7a46f6578a86b72f20acd7b0e55b1b4ebea09