[PATCH] nss: return ldap_user_extra_attrs attributes in origbyname request

[Sumit Bose]

Hi,

this two patches allow to have extra user attributes, like e.g. a
telephone number, for AD users of trusted domains on the IPA clients.

This first makes it possible that the attributes given in
ldap_user_extra_attrs can be read from AD in ipa-server-mode. The second
extends the list of attributes returned by sss_nss_getorigbyname().

bye,
Sumit

P.S. in the first patch I used sdap_extend_map_with_list() as it is used
in other places as well where the maps in the 4th and 6th argument are
the same. If I see it correctly we are leaking the old version of the
map this way. Since this currently happen only once during init it does
not do much harm, but maybe a ticket to fix this might be useful.

[Jakub Hrozek]

On Fri, Oct 24, 2014 at 09:46:12PM +0200, Sumit Bose wrote:
> Hi,
>
> this two patches allow to have extra user attributes, like e.g. a
> telephone number, for AD users of trusted domains on the IPA clients.
>
> This first makes it possible that the attributes given in
> ldap_user_extra_attrs can be read from AD in ipa-server-mode. The second
> extends the list of attributes returned by sss_nss_getorigbyname().
>
> bye,
> Sumit
>
> P.S. in the first patch I used sdap_extend_map_with_list() as it is used
> in other places as well where the maps in the 4th and 6th argument are
> the same. If I see it correctly we are leaking the old version of the
> map this way. Since this currently happen only once during init it does
> not do much harm, but maybe a ticket to fix this might be useful.

Ah, nice catch. Feel free to file the ticket, we should unit test that
function anyway and might as well include check_leaks_call to the test.
> From dedfb80621b379982ac0200a771d0d8b4fa78332 Mon Sep 17 00:00:00 2001
> From: Sumit Bose 
> Date: Fri, 24 Oct 2014 15:41:04 +0200
> Subject: [PATCH 1/2] IPA: inherit ldap_user_extra_attrs to AD subdomains
>
> Currently the component of the IPA provider which reads the AD user and
> group attributes in ipa-server-mode uses  default settings for the LDAP
> related attributes. As a result even if ldap_user_extra_attrs is defined
> in sssd.conf no extra attributes are read from AD.
>
> With the patch the value if ldap_user_extra_attrs is inherited to the AD
> subdomains to allow them to read extra attributes as well.
>
> Related to https://fedorahosted.org/sssd/ticket/2464

ACK
> From 704e0e0d1d9088e3bf4d4c29ac7f604396402244 Mon Sep 17 00:00:00 2001
> From: Sumit Bose 
> Date: Fri, 24 Oct 2014 11:30:33 +0200
> Subject: [PATCH 2/2] nss: return ldap_user_extra_attrs attributes in
>  origbyname request
>
> To allow IPA clients to offer special attributes of AD users form
> trusted domain the extdom plugin on the IPA server must send them to the
> clients. The extdom plugin already uses sss_nss_getorigbyname() to get
> attributes like the SID and the user principal name. This patch adds the
> attributes given the the ldap_user_extra_attrs option to the list of
> attributes returned by sss_nss_getorigbyname().
>
> Fixes https://fedorahosted.org/sssd/ticket/2464

Thank you, both patches work as advertized and I was able to retrieve a
custom attribute using the IFP respnder easily. I have some concerns,
though, see inline..
> src/responder/nss/nsssrv_cmd.c  | 170 ++++++++++++++++++++++++++++++++++------
>  src/tests/cmocka/test_nss_srv.c | 130 ++++++++++++++++++++++++++++++
>  2 files changed, 278 insertions(+), 22 deletions(-)
>
> diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
> index 9fca644..7481d49 100644
> --- a/src/responder/nss/nsssrv_cmd.c
> +++ b/src/responder/nss/nsssrv_cmd.c
> @@ -4131,6 +4131,7 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx)
>                             SYSDB_AD_ACCOUNT_EXPIRES,
>                             SYSDB_AD_USER_ACCOUNT_CONTROL,
>                             SYSDB_DEFAULT_ATTRS, NULL};
> const char *all_attrs[] = { "*", NULL};

Allowing the responder the retrieve /all/ attributes seems quite
dangerous to me, this allows to leak cachedPassword etc..

What is the use-case?
>  bool user_found = false;
>  bool group_found = false;
>  struct ldb_message *msg = NULL;
>
> @@ -4281,8 +4282,9 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx)
>              }
>          } else {
>              ret = sysdb_search_user_by_name(cmdctx, dom,
>                                        sysdb_name ? sysdb_name : name,
>                                        attrs, &msg);
>                    sysdb_name ? sysdb_name : name,
>                    cmdctx->cmd == SSS_NSS_GETORIGBYNAME ? all_attrs :attrs,
>                    &msg);
>        if (ret != EOK && ret != ENOENT) {
>            DEBUG(SSSDBG_CRIT_FAILURE,
>                  "Failed to make request to our cache!\n");
>
> @@ -4295,8 +4297,9 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx)
>              } else {
>                  talloc_free(msg);
>                  ret = sysdb_search_group_by_name(cmdctx, dom,
>                                             sysdb_name ? sysdb_name : name,
>                                             attrs, &msg);
>                   sysdb_name ? sysdb_name : name,
>                   cmdctx->cmd == SSS_NSS_GETORIGBYNAME ? all_attrs : attrs,
>                   &msg);
>            if (ret != EOK && ret != ENOENT) {
>                DEBUG(SSSDBG_CRIT_FAILURE,
>                      "Failed to make request to our cache!\n");
>
> @@ -4563,17 +4566,100 @@ static errno_t fill_sid(struct sss_packet *packet,
>      return EOK;
>  }
>
> +static errno_t get_extra_attribute_list(TALLOC_CTX *mem_ctx,
>                                    struct confdb_ctx *cdb,
>                                    struct sss_domain_info *domain,
>                                    size_t *new_attrs_count,
>                                    char ***new_attrs_list)
>
> +{
> int ret;
> TALLOC_CTX *tmp_ctx;
> char *confdb_path;
> char *extra_attrs;
> char **extra_attrs_list;
> size_t c;
> int count;
> char *sep;
> struct sss_domain_info *domain_head;
> tmp_ctx = talloc_new(NULL);
> if (tmp_ctx == NULL) {
>    DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n");
>    return ENOMEM;
> }
> domain_head = get_domains_head(domain);
> confdb_path = talloc_asprintf(tmp_ctx, CONFDB_DOMAIN_PATH_TMPL,
>                              domain_head->name);
> if (confdb_path == NULL) {
>    DEBUG(SSSDBG_OP_FAILURE, "talloc_asprintf failed.\n");
>    ret = ENOMEM;
>    goto done;
> }
> ret = confdb_get_string(cdb, tmp_ctx, confdb_path, "ldap_user_extra_attrs",
>                        NULL, &extra_attrs);

I don't like that we're leaking the knowledge about an LDAP attribute to
the responder. Can we use the 'user_attributes' parameter InfoPipe uses?
I know there would be one more step for the admin to configure, but I
would prefer that and keep the separated responder/backend architecture.
> if (ret != EOK) {
>    DEBUG(SSSDBG_OP_FAILURE, "confdb_get_string failed.\n");
>    goto done;
> }
> if (extra_attrs == NULL) {
>    DEBUG(SSSDBG_TRACE_FUNC,
>    "ldap_user_extra_attrs not set, nothing to do.\n");
>    ret = ENOENT;
>    goto done;
> }
> ret = split_on_separator(tmp_ctx, extra_attrs, ',', true, true,
>                         &extra_attrs_list, &count);
> if (ret != EOK || count < 0) {
>    DEBUG(SSSDBG_OP_FAILURE, "Failed to parse extra attributes!\n");
>    goto done;
> }
> for (c = 0; extra_attrs_list[c]; c++) {
>    sep = strchr(extra_attrs_list[c], ':');
>    if (sep != NULL) {
>        if (sep == extra_attrs_list[c] || *(sep + 1) == '\0') {
>            DEBUG(SSSDBG_CRIT_FAILURE,
>                  "Illegal ldap_user_extra_attrs entry found [%s].\n",
>                  extra_attrs_list[c]);
>            ret = EINVAL;
>            goto done;
>        }
>        *sep = '\0';
>    }
> }
> *new_attrs_count = count;
> *new_attrs_list = talloc_steal(mem_ctx, extra_attrs_list);
> ret = EOK;
>
> +done:
> talloc_free(tmp_ctx);
> return ret;
>
> +}
>
> static errno_t fill_orig(struct sss_packet *packet,
>                     struct resp_ctx *rctx,
>                     struct sss_domain_info *domain,
>                     enum sss_id_type id_type,
>                     struct ldb_message *msg)
>
> {
>      int ret;
> TALLOC_CTX *tmp_ctx;
> const char *tmp_str;
> uint8_t *body;
> size_t blen;
> size_t pctr = 0;
> size_t c;
> size_t sum;
> size_t found;
> size_t extra_attrs_count = 0;
> char **extra_attrs_list = NULL;
> const char *orig_attr_list[] = {SYSDB_SID_STR,
>                                 ORIGINALAD_PREFIX SYSDB_NAME,
>                                 ORIGINALAD_PREFIX SYSDB_UIDNUM,
>
> @@ -4586,42 +4672,81 @@ static errno_t fill_orig(struct sss_packet *packet,
>                                      SYSDB_AD_ACCOUNT_EXPIRES,
>                                      SYSDB_AD_USER_ACCOUNT_CONTROL,
>                                      NULL};
> struct sized_string keys[sizeof(orig_attr_list)];
> struct sized_string vals[sizeof(orig_attr_list)];
>
> struct sized_string *keys;
>
> struct sized_string *vals;
>
> tmp_ctx = talloc_new(NULL);
>
> if (tmp_ctx == NULL) {
>    DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n");
>    return ENOMEM;
>
> }
>
> ret = get_extra_attribute_list(tmp_ctx, rctx->cdb, domain,
>                               &extra_attrs_count, &extra_attrs_list);
>
> if (ret != EOK && ret != ENOENT) {
>    DEBUG(SSSDBG_OP_FAILURE, "get_extra_attribute_list failed.\n");
>    goto done;
>
> }
>
> keys = talloc_array(tmp_ctx, struct sized_string,
>                    sizeof(orig_attr_list) + extra_attrs_count);
>
> vals = talloc_array(tmp_ctx, struct sized_string,
>                    sizeof(orig_attr_list) + extra_attrs_count);
>
> if (keys == NULL || vals == NULL) {
>    DEBUG(SSSDBG_OP_FAILURE, "talloc_array failed.\n");
>    ret = ENOMEM;
>    goto done;
>
> }
>
> sum = 0;
>
> found = 0;
> for (c = 0; orig_attr_list[c] != NULL; c++) {
>     tmp_str = ldb_msg_find_attr_as_string(msg, orig_attr_list[c], NULL);
>     if (tmp_str != NULL) {
>        to_sized_string(&keys[c], orig_attr_list[c]);
>        sum += keys[c].len;
>        to_sized_string(&vals[c], tmp_str);
>        sum += vals[c].len;
>    } else {
>        vals[c].len = 0;
>        to_sized_string(&keys[found], orig_attr_list[c]);
>        sum += keys[found].len;
>        to_sized_string(&vals[found], tmp_str);
>        sum += vals[found].len;
>        found++;
>    }
>
> }
>
> for (c = 0; c < extra_attrs_count; c++) {
>    tmp_str = ldb_msg_find_attr_as_string(msg, extra_attrs_list[c], NULL);
>    if (tmp_str != NULL) {
>        to_sized_string(&keys[found], extra_attrs_list[c]);
>        sum += keys[found].len;
>        to_sized_string(&vals[found], tmp_str);
>        sum += vals[found].len;
>        found++;
>    }
>
> }
>
> ret = sss_packet_grow(packet, sum +  3 * sizeof(uint32_t));
>    if (ret != EOK) {
>        DEBUG(SSSDBG_OP_FAILURE, "sss_packet_grow failed.\n");
>    return ret;
>    goto done;
>
> }
>
> sss_packet_get_body(packet, &body, &blen);
>    SAFEALIGN_SETMEM_UINT32(body, 1, &pctr); /* Num results */
>    SAFEALIGN_SETMEM_UINT32(body + pctr, 0, &pctr); /* reserved */
>    SAFEALIGN_COPY_UINT32(body + pctr, &id_type, &pctr);
> for (c = 0; orig_attr_list[c] != NULL; c++) {
>    if (vals[c].len != 0) {
>        memcpy(&body[pctr], keys[c].str, keys[c].len);
>        pctr+= keys[c].len;
>        memcpy(&body[pctr], vals[c].str, vals[c].len);
>        pctr+= vals[c].len;
>    }
> for (c = 0; c < found; c++) {
>    memcpy(&body[pctr], keys[c].str, keys[c].len);
>    pctr+= keys[c].len;
>    memcpy(&body[pctr], vals[c].str, vals[c].len);
>    pctr+= vals[c].len;
> }
> return EOK;
> ret = EOK;
>
> +done:
> talloc_free(tmp_ctx);
> return ret;
>
> }
>
> static errno_t fill_name(struct sss_packet *packet,
> @@ -4789,7 +4914,8 @@ static errno_t nss_cmd_getbysid_send_reply(struct nss_dom_ctx *dctx)
>          ret = fill_sid(cctx->creq->out, id_type, dctx->res->msgs[0]);
>          break;
>      case SSS_NSS_GETORIGBYNAME:
>    ret = fill_orig(cctx->creq->out, id_type, dctx->res->msgs[0]);
>    ret = fill_orig(cctx->creq->out, cctx->rctx, dctx->domain, id_type,
>                    dctx->res->msgs[0]);
>    break;
> default:
>        DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported request type.\n");
>
> diff --git a/src/tests/cmocka/test_nss_srv.c b/src/tests/cmocka/test_nss_srv.c

Thanks a lot for the unit tests!

[Sumit Bose]

On Tue, Oct 28, 2014 at 05:37:25AM +0100, Jakub Hrozek wrote:
> On Fri, Oct 24, 2014 at 09:46:12PM +0200, Sumit Bose wrote:
> > Hi,
> >
> > this two patches allow to have extra user attributes, like e.g. a
> > telephone number, for AD users of trusted domains on the IPA clients.
> >
> > This first makes it possible that the attributes given in
> > ldap_user_extra_attrs can be read from AD in ipa-server-mode. The second
> > extends the list of attributes returned by sss_nss_getorigbyname().
> >
> > bye,
> > Sumit
> >
> > P.S. in the first patch I used sdap_extend_map_with_list() as it is used
> > in other places as well where the maps in the 4th and 6th argument are
> > the same. If I see it correctly we are leaking the old version of the
> > map this way. Since this currently happen only once during init it does
> > not do much harm, but maybe a ticket to fix this might be useful.
>
> Ah, nice catch. Feel free to file the ticket, we should unit test that
> function anyway and might as well include check_leaks_call to the test.

https://fedorahosted.org/freeipa/ticket/4667
>

...
> > src/responder/nss/nsssrv_cmd.c  | 170 ++++++++++++++++++++++++++++++++++------
> >  src/tests/cmocka/test_nss_srv.c | 130 ++++++++++++++++++++++++++++++
> >  2 files changed, 278 insertions(+), 22 deletions(-)
> >
> > diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
> > index 9fca644..7481d49 100644
> > --- a/src/responder/nss/nsssrv_cmd.c
> > +++ b/src/responder/nss/nsssrv_cmd.c
> > @@ -4131,6 +4131,7 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx)
> >                             SYSDB_AD_ACCOUNT_EXPIRES,
> >                             SYSDB_AD_USER_ACCOUNT_CONTROL,
> >                             SYSDB_DEFAULT_ATTRS, NULL};
> > const char *all_attrs[] = { "*", NULL};
>
> Allowing the responder the retrieve /all/ attributes seems quite
> dangerous to me, this allows to leak cachedPassword etc..
>
> What is the use-case?

My laziness :-). I changed this by using add_strings_lists() which I
send with the SSH pubkey patches.
> >  bool user_found = false;
> >  bool group_found = false;
> >  struct ldb_message *msg = NULL;
> >
> > @@ -4281,8 +4282,9 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx)
> >              }
> >          } else {
> >              ret = sysdb_search_user_by_name(cmdctx, dom,
> >                                        sysdb_name ? sysdb_name : name,
> >                                        attrs, &msg);
> >                    sysdb_name ? sysdb_name : name,
> >                    cmdctx->cmd == SSS_NSS_GETORIGBYNAME ? all_attrs :attrs,
> >                    &msg);
> >        if (ret != EOK && ret != ENOENT) {
> >            DEBUG(SSSDBG_CRIT_FAILURE,
> >                  "Failed to make request to our cache!\n");
> >
> > @@ -4295,8 +4297,9 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx)
> >              } else {
> >                  talloc_free(msg);
> >                  ret = sysdb_search_group_by_name(cmdctx, dom,
> >                                             sysdb_name ? sysdb_name : name,
> >                                             attrs, &msg);
> >                   sysdb_name ? sysdb_name : name,
> >                   cmdctx->cmd == SSS_NSS_GETORIGBYNAME ? all_attrs : attrs,
> >                   &msg);
> >            if (ret != EOK && ret != ENOENT) {
> >                DEBUG(SSSDBG_CRIT_FAILURE,
> >                      "Failed to make request to our cache!\n");
> >
> > @@ -4563,17 +4566,100 @@ static errno_t fill_sid(struct sss_packet *packet,
> >      return EOK;
> >  }
> >
> > +static errno_t get_extra_attribute_list(TALLOC_CTX *mem_ctx,
> >                                    struct confdb_ctx *cdb,
> >                                    struct sss_domain_info *domain,
> >                                    size_t *new_attrs_count,
> >                                    char ***new_attrs_list)
> >
> > +{
> > int ret;
> > TALLOC_CTX *tmp_ctx;
> > char *confdb_path;
> > char *extra_attrs;
> > char **extra_attrs_list;
> > size_t c;
> > int count;
> > char *sep;
> > struct sss_domain_info *domain_head;
> > tmp_ctx = talloc_new(NULL);
> > if (tmp_ctx == NULL) {
> >    DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n");
> >    return ENOMEM;
> > }
> > domain_head = get_domains_head(domain);
> > confdb_path = talloc_asprintf(tmp_ctx, CONFDB_DOMAIN_PATH_TMPL,
> >                              domain_head->name);
> > if (confdb_path == NULL) {
> >    DEBUG(SSSDBG_OP_FAILURE, "talloc_asprintf failed.\n");
> >    ret = ENOMEM;
> >    goto done;
> > }
> > ret = confdb_get_string(cdb, tmp_ctx, confdb_path, "ldap_user_extra_attrs",
> >                        NULL, &extra_attrs);
>
> I don't like that we're leaking the knowledge about an LDAP attribute to
> the responder. Can we use the 'user_attributes' parameter InfoPipe uses?
> I know there would be one more step for the admin to configure, but I
> would prefer that and keep the separated responder/backend architecture.

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

[Jakub Hrozek]

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?

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.

[Sumit Bose]

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.
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Lukas Slebodnik]

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 
> 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

[Lukas Slebodnik]

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 
> > 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

[Jakub Hrozek]

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 
> > > 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.

[Jakub Hrozek]

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 
> > > > 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.

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:[e-mail] array:string:name,gecos,phone
method return sender=:1.27 -> dest=:1.29 reply_serial=2
   array [
      dict entry(
         string "name"
         variant             array [
               string "[e-mail]"
            ]
      )
      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.

[Sumit Bose]

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 
> > > > > 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:[e-mail] array:string:name,gecos,phone
> method return sender=:1.27 -> dest=:1.29 reply_serial=2
>    array [
>       dict entry(
>          string "name"
>          variant             array [
>                string "[e-mail]"
>             ]
>       )
>       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
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

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 
> > > > > > 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:[e-mail] array:string:name,gecos,phone
> > method return sender=:1.27 -> dest=:1.29 reply_serial=2
> >    array [
> >       dict entry(
> >          string "name"
> >          variant             array [
> >                string "[e-mail]"
> >             ]
> >       )
> >       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
> > [e-mail]
> > 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;

And push, I don't have any other comments.

[Sumit Bose]

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 
> > > > > > >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:[e-mail] array:string:name,gecos,phone
> > > method return sender=:1.27 -> dest=:1.29 reply_serial=2
> > >    array [
> > >       dict entry(
> > >          string "name"
> > >          variant             array [
> > >                string "[e-mail]"
> > >             ]
> > >       )
> > >       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
> > > [e-mail]
> > > 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
> And push, I don't have any other comments.
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Wed, Nov 05, 2014 at 11:46:16AM +0100, Sumit Bose wrote:
> 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 
> > > > > > >>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:[e-mail] array:string:name,gecos,phone
> > > > method return sender=:1.27 -> dest=:1.29 reply_serial=2
> > > >    array [
> > > >       dict entry(
> > > >          string "name"
> > > >          variant             array [
> > > >                string "[e-mail]"
> > > >             ]
> > > >       )
> > > >       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
> > > > [e-mail]
> > > > 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