[PATCHES] cache_req improvements

[Pavel Březina]

Hi,
these patches were created as part of my dbus work however they do not 
depend on it. Those patches can be applied on top of current master and 
review separately.

Recently introduced cache_req interface which hides all the logic of 
search for a sysdb object in cache and data provider supported only user 
by name and initgroups request.

These patches add support also for "user by id", "group by name", "group 
by id" (for new Users and Groups IFP interfaces). It also hides 
sss_parse_inp into cache_req so the may no longer be the need to parse 
username in consumers of the API.

Everything is unit tested.

Happy reviewing :-)

[Pavel Březina]

On 01/27/2015 02:19 PM, Pavel Březina wrote:
> Hi,
> these patches were created as part of my dbus work however they do not
> depend on it. Those patches can be applied on top of current master and
> review separately.
>
> Recently introduced cache_req interface which hides all the logic of
> search for a sysdb object in cache and data provider supported only user
> by name and initgroups request.
>
> These patches add support also for "user by id", "group by name", "group
> by id" (for new Users and Groups IFP interfaces). It also hides
> sss_parse_inp into cache_req so the may no longer be the need to parse
> username in consumers of the API.
>
> Everything is unit tested.
>
> Happy reviewing :-)

Self nack. I was passing wrong name (input->name instead of 
input->orig_name) into sss_parse_inp which caused SIGSEGV. This was not 
caught by unit tests since the return value of sss_parse_inp is mocked.

I fixed this and renamed input->name into input->safe_name to avoid 
another name confusion.

[Jakub Hrozek]

On Fri, Jan 30, 2015 at 05:04:09PM +0100, Pavel Březina wrote:
> On 01/27/2015 02:19 PM, Pavel Březina wrote:
> > Hi,
> > these patches were created as part of my dbus work however they do not
> > depend on it. Those patches can be applied on top of current master and
> > review separately.
> >
> > Recently introduced cache_req interface which hides all the logic of
> > search for a sysdb object in cache and data provider supported only user
> > by name and initgroups request.
> >
> > These patches add support also for "user by id", "group by name", "group
> > by id" (for new Users and Groups IFP interfaces). It also hides
> > sss_parse_inp into cache_req so the may no longer be the need to parse
> > username in consumers of the API.
> >
> > Everything is unit tested.
> >
> > Happy reviewing :-)
>
> Self nack. I was passing wrong name (input->name instead of
> input->orig_name) into sss_parse_inp which caused SIGSEGV. This was not
> caught by unit tests since the return value of sss_parse_inp is mocked.
>
> I fixed this and renamed input->name into input->safe_name to avoid another
> name confusion.
> From 2dc53a8123ac6db65a1138e0dbdba426c5604acd Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Mon, 26 Jan 2015 11:17:24 +0100
> Subject: [PATCH 01/13] cache_req tests: rename test_user to test_user_by_name
>
> This is done in order to distinguish those tests from other
> user tests that are about to come. For example: test_user_by_id.

ACK
> From b50fd0281ea83a69830bd82101c83e4b3eae71b0 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Mon, 26 Jan 2015 15:28:50 +0100
> Subject: [PATCH 02/13] cache_req tests: define user name constant
>
> Using a constant here is better since the name is shared between
> the test function and testing _done function.
>
> src/tests/cmocka/test_responder_cache_req.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/src/tests/cmocka/test_responder_cache_req.c b/src/tests/cmocka/test_responder_cache_req.c
> index 6c83cac0fb9bee6ebe767b5cca14622cce2db23c..bc551f8a3518aa777febd19cfe3947868a19b39c 100644
> --- a/src/tests/cmocka/test_responder_cache_req.c
> +++ b/src/tests/cmocka/test_responder_cache_req.c
> @@ -33,6 +33,8 @@
>  #define TEST_DOM_NAME "responder_cache_req_test"
>  #define TEST_ID_PROVIDER "ldap"
>
> +#define TEST_USER_NAME "test-user"
>
> #define new_single_domain_test(test) \
>      unit_test_setup_teardown(test_ ## test, \
>                               test_single_domain_setup, \
> @@ -86,9 +88,10 @@ __wrap_sss_dp_get_account_send(TALLOC_CTX *mem_ctx,
>      ctx->dp_called = true;
>  if (ctx->create_user) {
>    ret = sysdb_store_user(ctx->tctx->dom, "test-user", "pwd", 1000, 1000,
>                           NULL, NULL, NULL, "cn=test-user,dc=test", NULL,
>                           NULL, 1000, time(NULL));
>    ret = sysdb_store_user(ctx->tctx->dom, TEST_USER_NAME, "pwd",
>                           1000, 1000, NULL, NULL, NULL,
>                           "cn=test-user,dc=test", NULL, NULL,

You can also use the #define here:
    "cn="TEST_USER_NAME",dc=test"

but it's fine if you don't, your call.
> From db6c8c38bd450f21a9f8d0489fc2b88b6cf3d1b9 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Fri, 23 Jan 2015 14:03:07 +0100
> Subject: [PATCH 03/13] cache_req: preparations for different input type
>
> Currently cache_req takes only user name as an input parameter. However,
> this is not enough since we will need also UID and GID in the future.
> This patch creates a structure to hold input parameters so it can be
> simply extended to support other input types.

General comment - it would be great if the new code could be decorated
with DEBUG statements, not only for failures, but also in order for us
to follow the general flow. Currently it would be really hard to tell how
far we got and what went wrong.

I'm fine with this being an extra patch atop the small changes, but the
DEBUG messages must be there in order for the patchset to be used,
especially by other responders.
> src/responder/common/responder_cache_req.c | 341 +++++++++++++++++++++--------
>  src/responder/common/responder_cache_req.h |  41 +++-
>  src/responder/ifp/ifpsrv_cmd.c             |  24 +-
>  3 files changed, 306 insertions(+), 100 deletions(-)
>
> diff --git a/src/responder/common/responder_cache_req.c b/src/responder/common/responder_cache_req.c
> index 39f90cfe16fa7c726eae92110ce717ffefb556a2..b10dfbd783be1c8f4782f0b765644ca1083749a7 100644
> --- a/src/responder/common/responder_cache_req.c
> +++ b/src/responder/common/responder_cache_req.c
> @@ -28,54 +28,175 @@
>  #include "responder/common/responder_cache_req.h"
>  #include "providers/data_provider.h"
>
> -static errno_t cache_req_check_ncache(enum sss_dp_acct_type dp_type,
> +struct cache_req_input {
> enum cache_req_type type;
> /* Provided input. */
> const char *orig_name;
> /* Data Provider request type resolved from @type. */
> enum sss_dp_acct_type dp_type;

I'm not sure why we have both enum cache_req_type and sss_dp_acct_type
with 1:1 mapping used at the same abstraction layer. Either
cache_req_type is something private to the cache_req code and then we
should always use it except when sending requests to the DP where we
should do a one-time conversion, or we can always use sss_dp_acct_type.

Maybe add another patch that replaces dp_type with type for internal
branching within this module?
> /* Domain related informations. */
> struct sss_domain_info *domain;
> /* Name sanitized according to domain rules such as case sensitivity and
> * replacement of space character. This needs to be set up for each
> * domain separately. */
> const char *safe_name;
> /* Fully qualified object name. */
> const char *fqn;
>
> +};
>
> +struct cache_req_input *
> +cache_req_input_create(TALLOC_CTX *mem_ctx,
>                   enum cache_req_type type,
>                   const char *name)
>
> +{
> struct cache_req_input *input;
> input = talloc_zero(mem_ctx, struct cache_req_input);
> if (input == NULL) {
>    return NULL;
> }
> input->type = type;
> /* Check that input parameters match selected type. */
> switch (input->type) {
> case CACHE_REQ_USER_BY_NAME:
> case CACHE_REQ_INITGROUPS:
>    if (name == NULL) {
>        DEBUG(SSSDBG_CRIT_FAILURE, "Bug: name cannot be NULL!\n");
>        goto fail;
>    }
>    input->orig_name = talloc_strdup(input, name);
>    if (input->orig_name == NULL) {
>        goto fail;
>    }
>    break;
> }
> /* Resolve Data Provider request type. */
> switch (type) {
> case CACHE_REQ_USER_BY_NAME:
>    input->dp_type = SSS_DP_USER;
>    break;
> case CACHE_REQ_INITGROUPS:
>    input->dp_type = SSS_DP_INITGROUPS;
>    break;
> }

This function looks OK except I would like the dp_type to go away :)
> return input;
>
> +fail:
> talloc_free(input);
> return NULL;
>
> +}
>
> +static errno_t
> +cache_req_input_set_domain(struct cache_req_input *input,
>                       struct sss_domain_info *domain,
>                       struct resp_ctx *rctx)
>
> +{
> TALLOC_CTX *tmp_ctx = NULL;
> const char *name = NULL;
> const char *fqn = NULL;
> errno_t ret;
> tmp_ctx = talloc_new(NULL);
> if (tmp_ctx == NULL) {
>    return ENOMEM;
> }
> talloc_zfree(input->safe_name);
> talloc_zfree(input->fqn);

In general I like the approach you've taken but I think we can make it
even better. Add another structure like cache_req_dom_ctx and add it inside
the cache_req_input structure. Always create cache_req_dom_ctx as a
talloc child of cache_req_input.

This would not only collapse the above talloc_frees into one, but it
would also nicely separate the per-domain data from per-lookup data and
avoid mixing data from different domains.

I also think dom_name sounds better than safe_name, but meh.
> switch (input->type) {
> case CACHE_REQ_USER_BY_NAME:
> case CACHE_REQ_INITGROUPS:
>    name = sss_get_cased_name(tmp_ctx, input->orig_name,
>                              domain->case_sensitive);
>    if (name == NULL) {
>        ret = ENOMEM;
>        goto done;
>    }
>    name = sss_reverse_replace_space(tmp_ctx, name, rctx->override_space);
>    if (name == NULL) {
>        ret = ENOMEM;
>        goto done;
>    }
>    fqn = talloc_asprintf(tmp_ctx, "%s@%s", name, domain->name);

I'm afraid FQDN is configurable in SSSD, so creating it is not this easy..
>    if (fqn == NULL) {
>        ret = ENOMEM;
>        goto done;
>    }
>    break;
> }
> input->domain = domain;
> input->safe_name = talloc_steal(input, name);
> input->fqn = talloc_steal(input, fqn);
> ret = EOK;
>
> +done:
> talloc_free(tmp_ctx);
> return ret;
>
> +}

[...]
> static errno_t cache_req_get_object(TALLOC_CTX *mem_ctx,
>                                enum sss_dp_acct_type dp_type,
>                                struct sss_domain_info *domain,
>                                const char *name,
>                                struct cache_req_input *input,
>                                struct ldb_result **_result)
>
> {
>      struct ldb_result *result = NULL;
>      bool one_item_only;
>      errno_t ret;
> DEBUG(SSSDBG_FUNC_DATA, "Requesting info for [%s@%s]\n",
>      name, domain->name);
> DEBUG(SSSDBG_FUNC_DATA, "Requesting info for [%s]\n", input->fqn);
> switch (dp_type) {
> case SSS_DP_USER:
> switch (input->type) {
> case CACHE_REQ_USER_BY_NAME:
>     one_item_only = true;
>    ret = sysdb_getpwnam_with_views(mem_ctx, domain, name, &result);
>    ret = sysdb_getpwnam_with_views(mem_ctx, input->domain,
>                                    input->safe_name, &result);
>    break;
> case SSS_DP_INITGROUPS:
> case CACHE_REQ_INITGROUPS:
>     one_item_only = false;
>    ret = sysdb_initgroups_with_views(mem_ctx, domain, name, &result);
>    ret = sysdb_initgroups_with_views(mem_ctx, input->domain,
>                                      input->safe_name, &result);
>    break;
> default:
>        ret = EINVAL;
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported DP request type\n");
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported cache request type\n");
>    break;

Should we free result if ret != EOK ?

This is as far as I've got today, I'll continue with review tomorrow.

[Pavel Březina]

On 02/11/2015 09:13 PM, Jakub Hrozek wrote:
> On Fri, Jan 30, 2015 at 05:04:09PM +0100, Pavel Březina wrote:
> > On 01/27/2015 02:19 PM, Pavel Březina wrote:
> > > Hi,
> > > these patches were created as part of my dbus work however they do not
> > > depend on it. Those patches can be applied on top of current master and
> > > review separately.
> > >
> > > Recently introduced cache_req interface which hides all the logic of
> > > search for a sysdb object in cache and data provider supported only user
> > > by name and initgroups request.
> > >
> > > These patches add support also for "user by id", "group by name", "group
> > > by id" (for new Users and Groups IFP interfaces). It also hides
> > > sss_parse_inp into cache_req so the may no longer be the need to parse
> > > username in consumers of the API.
> > >
> > > Everything is unit tested.
> > >
> > > Happy reviewing :-)
> >
> > Self nack. I was passing wrong name (input->name instead of
> > input->orig_name) into sss_parse_inp which caused SIGSEGV. This was not
> > caught by unit tests since the return value of sss_parse_inp is mocked.
> >
> > I fixed this and renamed input->name into input->safe_name to avoid another
> > name confusion.
> > From 2dc53a8123ac6db65a1138e0dbdba426c5604acd Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Mon, 26 Jan 2015 11:17:24 +0100
> > Subject: [PATCH 01/13] cache_req tests: rename test_user to test_user_by_name
> >
> > This is done in order to distinguish those tests from other
> > user tests that are about to come. For example: test_user_by_id.
>
> ACK
> > From b50fd0281ea83a69830bd82101c83e4b3eae71b0 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Mon, 26 Jan 2015 15:28:50 +0100
> > Subject: [PATCH 02/13] cache_req tests: define user name constant
> >
> > Using a constant here is better since the name is shared between
> > the test function and testing _done function.
> >
> > src/tests/cmocka/test_responder_cache_req.c | 25 ++++++++++++++-----------
> >   1 file changed, 14 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/tests/cmocka/test_responder_cache_req.c b/src/tests/cmocka/test_responder_cache_req.c
> > index 6c83cac0fb9bee6ebe767b5cca14622cce2db23c..bc551f8a3518aa777febd19cfe3947868a19b39c 100644
> > --- a/src/tests/cmocka/test_responder_cache_req.c
> > +++ b/src/tests/cmocka/test_responder_cache_req.c
> > @@ -33,6 +33,8 @@
> >   #define TEST_DOM_NAME "responder_cache_req_test"
> >   #define TEST_ID_PROVIDER "ldap"
> >
> > +#define TEST_USER_NAME "test-user"
> > #define new_single_domain_test(test) \
> >     unit_test_setup_teardown(test_ ## test, \
> >                              test_single_domain_setup, \
> >
> > @@ -86,9 +88,10 @@ __wrap_sss_dp_get_account_send(TALLOC_CTX *mem_ctx,
> >       ctx->dp_called = true;
> >   if (ctx->create_user) {
> >    ret = sysdb_store_user(ctx->tctx->dom, "test-user", "pwd", 1000, 1000,
> >                           NULL, NULL, NULL, "cn=test-user,dc=test", NULL,
> >                           NULL, 1000, time(NULL));
> >    ret = sysdb_store_user(ctx->tctx->dom, TEST_USER_NAME, "pwd",
> >                           1000, 1000, NULL, NULL, NULL,
> >                           "cn=test-user,dc=test", NULL, NULL,
>
> You can also use the #define here:
>      "cn="TEST_USER_NAME",dc=test"
>
> but it's fine if you don't, your call.
> > From db6c8c38bd450f21a9f8d0489fc2b88b6cf3d1b9 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Fri, 23 Jan 2015 14:03:07 +0100
> > Subject: [PATCH 03/13] cache_req: preparations for different input type
> >
> > Currently cache_req takes only user name as an input parameter. However,
> > this is not enough since we will need also UID and GID in the future.
> > This patch creates a structure to hold input parameters so it can be
> > simply extended to support other input types.
>
> General comment - it would be great if the new code could be decorated
> with DEBUG statements, not only for failures, but also in order for us
> to follow the general flow. Currently it would be really hard to tell how
> far we got and what went wrong.
>
> I'm fine with this being an extra patch atop the small changes, but the
> DEBUG messages must be there in order for the patchset to be used,
> especially by other responders.
> > src/responder/common/responder_cache_req.c | 341 +++++++++++++++++++++--------
> >   src/responder/common/responder_cache_req.h |  41 +++-
> >   src/responder/ifp/ifpsrv_cmd.c             |  24 +-
> >   3 files changed, 306 insertions(+), 100 deletions(-)
> >
> > diff --git a/src/responder/common/responder_cache_req.c b/src/responder/common/responder_cache_req.c
> > index 39f90cfe16fa7c726eae92110ce717ffefb556a2..b10dfbd783be1c8f4782f0b765644ca1083749a7 100644
> > --- a/src/responder/common/responder_cache_req.c
> > +++ b/src/responder/common/responder_cache_req.c
> > @@ -28,54 +28,175 @@
> >   #include "responder/common/responder_cache_req.h"
> >   #include "providers/data_provider.h"
> >
> > -static errno_t cache_req_check_ncache(enum sss_dp_acct_type dp_type,
> > +struct cache_req_input {
> > enum cache_req_type type;
> > /* Provided input. */
> > const char *orig_name;
> > /* Data Provider request type resolved from @type. */
> > enum sss_dp_acct_type dp_type;
>
> I'm not sure why we have both enum cache_req_type and sss_dp_acct_type
> with 1:1 mapping used at the same abstraction layer. Either
> cache_req_type is something private to the cache_req code and then we
> should always use it except when sending requests to the DP where we
> should do a one-time conversion, or we can always use sss_dp_acct_type.
>
> Maybe add another patch that replaces dp_type with type for internal
> branching within this module?

The cache_req_type is used to distinguish between get_user_by_name, 
get_user_by_id, get_initgroups call which all maps to DP_USER call to 
provider. So the sss_dp_acct_type is not sufficient because we need 
larger granularity but at the same time we can't get rid of if that 
easily unless the dp interface is changed. This may be a future 
enhancement when other responders use this.
> > /* Domain related informations. */
> > struct sss_domain_info *domain;
> > /* Name sanitized according to domain rules such as case sensitivity and
> > * replacement of space character. This needs to be set up for each
> > * domain separately. */
> > const char *safe_name;
> > /* Fully qualified object name. */
> > const char *fqn;
> >
> > +};
> >
> > +struct cache_req_input *
> > +cache_req_input_create(TALLOC_CTX *mem_ctx,
> >                   enum cache_req_type type,
> >                   const char *name)
> >
> > +{
> > struct cache_req_input *input;
> > input = talloc_zero(mem_ctx, struct cache_req_input);
> > if (input == NULL) {
> >    return NULL;
> > }
> > input->type = type;
> > /* Check that input parameters match selected type. */
> > switch (input->type) {
> > case CACHE_REQ_USER_BY_NAME:
> > case CACHE_REQ_INITGROUPS:
> >    if (name == NULL) {
> >        DEBUG(SSSDBG_CRIT_FAILURE, "Bug: name cannot be NULL!\n");
> >        goto fail;
> >    }
> >    input->orig_name = talloc_strdup(input, name);
> >    if (input->orig_name == NULL) {
> >        goto fail;
> >    }
> >    break;
> > }
> > /* Resolve Data Provider request type. */
> > switch (type) {
> > case CACHE_REQ_USER_BY_NAME:
> >    input->dp_type = SSS_DP_USER;
> >    break;
> > case CACHE_REQ_INITGROUPS:
> >    input->dp_type = SSS_DP_INITGROUPS;
> >    break;
> > }
>
> This function looks OK except I would like the dp_type to go away :)
> > return input;
> >
> > +fail:
> > talloc_free(input);
> > return NULL;
> >
> > +}
> >
> > +static errno_t
> > +cache_req_input_set_domain(struct cache_req_input *input,
> >                       struct sss_domain_info *domain,
> >                       struct resp_ctx *rctx)
> >
> > +{
> > TALLOC_CTX *tmp_ctx = NULL;
> > const char *name = NULL;
> > const char *fqn = NULL;
> > errno_t ret;
> > tmp_ctx = talloc_new(NULL);
> > if (tmp_ctx == NULL) {
> >    return ENOMEM;
> > }
> > talloc_zfree(input->safe_name);
> > talloc_zfree(input->fqn);
>
> In general I like the approach you've taken but I think we can make it
> even better. Add another structure like cache_req_dom_ctx and add it inside
> the cache_req_input structure. Always create cache_req_dom_ctx as a
> talloc child of cache_req_input.
>
> This would not only collapse the above talloc_frees into one, but it
> would also nicely separate the per-domain data from per-lookup data and
> avoid mixing data from different domains.
>
> I also think dom_name sounds better than safe_name, but meh.
> > switch (input->type) {
> > case CACHE_REQ_USER_BY_NAME:
> > case CACHE_REQ_INITGROUPS:
> >    name = sss_get_cased_name(tmp_ctx, input->orig_name,
> >                              domain->case_sensitive);
> >    if (name == NULL) {
> >        ret = ENOMEM;
> >        goto done;
> >    }
> >    name = sss_reverse_replace_space(tmp_ctx, name, rctx->override_space);
> >    if (name == NULL) {
> >        ret = ENOMEM;
> >        goto done;
> >    }
> >    fqn = talloc_asprintf(tmp_ctx, "%s@%s", name, domain->name);
>
> I'm afraid FQDN is configurable in SSSD, so creating it is not this easy..

This is just for purpose of debug messages, so its ok imho. It is used 
in sssd everywhere.
> >    if (fqn == NULL) {
> >        ret = ENOMEM;
> >        goto done;
> >    }
> >    break;
> > }
> > input->domain = domain;
> > input->safe_name = talloc_steal(input, name);
> > input->fqn = talloc_steal(input, fqn);
> > ret = EOK;
> >
> > +done:
> > talloc_free(tmp_ctx);
> > return ret;
> >
> > +}
>
> [...]
> > static errno_t cache_req_get_object(TALLOC_CTX *mem_ctx,
> >                                enum sss_dp_acct_type dp_type,
> >                                struct sss_domain_info *domain,
> >                                const char *name,
> >                                struct cache_req_input *input,
> >                                 struct ldb_result **_result)
> > {
> >     struct ldb_result *result = NULL;
> >     bool one_item_only;
> >     errno_t ret;
> > DEBUG(SSSDBG_FUNC_DATA, "Requesting info for [%s@%s]\n",
> >      name, domain->name);
> > DEBUG(SSSDBG_FUNC_DATA, "Requesting info for [%s]\n", input->fqn);
> > switch (dp_type) {
> > case SSS_DP_USER:
> > switch (input->type) {
> > case CACHE_REQ_USER_BY_NAME:
> >      one_item_only = true;
> >    ret = sysdb_getpwnam_with_views(mem_ctx, domain, name, &result);
> >    ret = sysdb_getpwnam_with_views(mem_ctx, input->domain,
> >                                    input->safe_name, &result);
> >     break;
> > case SSS_DP_INITGROUPS:
> > case CACHE_REQ_INITGROUPS:
> >      one_item_only = false;
> >    ret = sysdb_initgroups_with_views(mem_ctx, domain, name, &result);
> >    ret = sysdb_initgroups_with_views(mem_ctx, input->domain,
> >                                      input->safe_name, &result);
> >     break;
> > default:
> >     ret = EINVAL;
> >    DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported DP request type\n");
> >    DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported cache request type\n");
> >     break;
>
> Should we free result if ret != EOK ?

Hmm, probably yes.
> This is as far as I've got today, I'll continue with review tomorrow.

[Jakub Hrozek]

On Thu, Feb 12, 2015 at 10:07:57AM +0100, Pavel Březina wrote:
> > I'm not sure why we have both enum cache_req_type and sss_dp_acct_type
> > with 1:1 mapping used at the same abstraction layer. Either
> > cache_req_type is something private to the cache_req code and then we
> > should always use it except when sending requests to the DP where we
> > should do a one-time conversion, or we can always use sss_dp_acct_type.
> >
> > Maybe add another patch that replaces dp_type with type for internal
> > branching within this module?
>
> The cache_req_type is used to distinguish between get_user_by_name,
> get_user_by_id, get_initgroups call which all maps to DP_USER call to
> provider. So the sss_dp_acct_type is not sufficient because we need larger
> granularity but at the same time we can't get rid of if that easily unless
> the dp interface is changed. This may be a future enhancement when other
> responders use this.

OK, can you add a note to the code that the member should be removed
later? Just a FIXME would do.

[...]
> > In general I like the approach you've taken but I think we can make it
> > even better. Add another structure like cache_req_dom_ctx and add it inside
> > the cache_req_input structure. Always create cache_req_dom_ctx as a
> > talloc child of cache_req_input.
> >
> > This would not only collapse the above talloc_frees into one, but it
> > would also nicely separate the per-domain data from per-lookup data and
> > avoid mixing data from different domains.
> >
> > I also think dom_name sounds better than safe_name, but meh.

This is my main point in the whole patchset, I think.
> > > switch (input->type) {
> > > case CACHE_REQ_USER_BY_NAME:
> > > case CACHE_REQ_INITGROUPS:
> > >    name = sss_get_cased_name(tmp_ctx, input->orig_name,
> > >                              domain->case_sensitive);
> > >    if (name == NULL) {
> > >        ret = ENOMEM;
> > >        goto done;
> > >    }
> > >    name = sss_reverse_replace_space(tmp_ctx, name, rctx->override_space);
> > >    if (name == NULL) {
> > >        ret = ENOMEM;
> > >        goto done;
> > >    }
> > >    fqn = talloc_asprintf(tmp_ctx, "%s@%s", name, domain->name);
> >
> > I'm afraid FQDN is configurable in SSSD, so creating it is not this easy..
>
> This is just for purpose of debug messages, so its ok imho. It is used in
> sssd everywhere.

OK, then would it be too much work to rename the variable to something
like log_fqn or debug_fqn? My concern is that 2 years later someone
would use this data for output, not realizing the data is hardcoded.

[...]
> > > static errno_t cache_req_get_object(TALLOC_CTX *mem_ctx,
> > >                                enum sss_dp_acct_type dp_type,
> > >                                struct sss_domain_info *domain,
> > >                                const char *name,
> > >                                struct cache_req_input *input,
> > >                                struct ldb_result **_result)
> > >
> > > {
> > >      struct ldb_result *result = NULL;
> > >      bool one_item_only;
> > >      errno_t ret;
> > > DEBUG(SSSDBG_FUNC_DATA, "Requesting info for [%s@%s]\n",
> > >      name, domain->name);
> > > DEBUG(SSSDBG_FUNC_DATA, "Requesting info for [%s]\n", input->fqn);
> > > switch (dp_type) {
> > > case SSS_DP_USER:
> > > switch (input->type) {
> > > case CACHE_REQ_USER_BY_NAME:
> > >     one_item_only = true;
> > >    ret = sysdb_getpwnam_with_views(mem_ctx, domain, name, &result);
> > >    ret = sysdb_getpwnam_with_views(mem_ctx, input->domain,
> > >                                    input->safe_name, &result);
> > >    break;
> > > case SSS_DP_INITGROUPS:
> > > case CACHE_REQ_INITGROUPS:
> > >     one_item_only = false;
> > >    ret = sysdb_initgroups_with_views(mem_ctx, domain, name, &result);
> > >    ret = sysdb_initgroups_with_views(mem_ctx, input->domain,
> > >                                      input->safe_name, &result);
> > >    break;
> > > default:
> > >        ret = EINVAL;
> > >    DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported DP request type\n");
> > >    DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported cache request type\n");
> > >    break;
> >
> > Should we free result if ret != EOK ?
>
> Hmm, probably yes.
> > This is as far as I've got today, I'll continue with review tomorrow.

I will send a review of the other patches separately.

[Jakub Hrozek]

On Fri, Jan 30, 2015 at 05:04:09PM +0100, Pavel Březina wrote:
> On 01/27/2015 02:19 PM, Pavel Březina wrote:
> > Hi,
> > these patches were created as part of my dbus work however they do not
> > depend on it. Those patches can be applied on top of current master and
> > review separately.
> >
> > Recently introduced cache_req interface which hides all the logic of
> > search for a sysdb object in cache and data provider supported only user
> > by name and initgroups request.
> >
> > These patches add support also for "user by id", "group by name", "group
> > by id" (for new Users and Groups IFP interfaces). It also hides
> > sss_parse_inp into cache_req so the may no longer be the need to parse
> > username in consumers of the API.
> >
> > Everything is unit tested.
> >
> > Happy reviewing :-)
>
> Self nack. I was passing wrong name (input->name instead of
> input->orig_name) into sss_parse_inp which caused SIGSEGV. This was not
> caught by unit tests since the return value of sss_parse_inp is mocked.
>
> I fixed this and renamed input->name into input->safe_name to avoid another
> name confusion.
> From db6c8c38bd450f21a9f8d0489fc2b88b6cf3d1b9 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Fri, 23 Jan 2015 14:03:07 +0100
> Subject: [PATCH 03/13] cache_req: preparations for different input type
>
> Currently cache_req takes only user name as an input parameter. However,
> this is not enough since we will need also UID and GID in the future.
> This patch creates a structure to hold input parameters so it can be
> simply extended to support other input types.
>
> src/responder/common/responder_cache_req.c | 341 +++++++++++++++++++++--------
>  src/responder/common/responder_cache_req.h |  41 +++-
>  src/responder/ifp/ifpsrv_cmd.c             |  24 +-
>  3 files changed, 306 insertions(+), 100 deletions(-)

[...]
> static errno_t cache_req_get_object(TALLOC_CTX *mem_ctx,
>                                enum sss_dp_acct_type dp_type,
>                                struct sss_domain_info *domain,
>                                const char *name,
>                                struct cache_req_input *input,
>                                struct ldb_result **_result)
>
> {
>      struct ldb_result *result = NULL;
>      bool one_item_only;

Not related to this patchset per se but I noticed this function returns
ENOENT if one_item_only is set but multiple objects match. I think this
should be a hard error.

[...]
> @@ -260,7 +357,7 @@ static errno_t cache_req_cache_check(struct tevent_req *req)
>      if (state->result == NULL || state->result->count == 0) {
>          ret = ENOENT;
>      } else {
>    if (state->dp_type == SSS_DP_INITGROUPS) {
>    if (state->input->dp_type == SSS_DP_INITGROUPS) {

Could we for example remove dp_type here and only use the internal type?

[...]
> --- a/src/responder/ifp/ifpsrv_cmd.c
> +++ b/src/responder/ifp/ifpsrv_cmd.c
> @@ -477,6 +477,7 @@ static void
>  ifp_user_get_attr_lookup(struct tevent_req *subreq)
>  {
>      struct ifp_user_get_attr_state *state = NULL;
> struct cache_req_input *input = NULL;
> struct tevent_req *req = NULL;
> errno_t ret;
>
> @@ -490,9 +491,30 @@ ifp_user_get_attr_lookup(struct tevent_req *subreq)
>          return;
>      }
> switch (state->search_type) {
> case SSS_DP_USER:
>    input = cache_req_input_create(state, CACHE_REQ_USER_BY_NAME,
>                                   state->name);
>    break;
> case SSS_DP_INITGROUPS:
>    input = cache_req_input_create(state, CACHE_REQ_INITGROUPS,
>                                   state->name);

Why don't you use initgr_by_name() directly?
>    break;
> default:
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported search type [%d]!\n",
>          state->search_type);
>    tevent_req_error(req, ERR_INTERNAL);
>    return;
> }
> if (input == NULL) {
>    tevent_req_error(req, ENOMEM);
>    return;
> }
> subreq = cache_req_send(state, state->rctx->ev, state->rctx,
>                            state->ncache, state->neg_timeout, 0,
>                        state->search_type, state->domname, state->name);
>                        state->domname, input);
> if (subreq == NULL) {
>        tevent_req_error(req, ENOMEM);
>        return;
>
> -- 
> 1.7.11.7
> From d1e87b6a4adc80a99a8df5ca02dd33c3a2dd68b1 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Fri, 23 Jan 2015 15:32:55 +0100
> Subject: [PATCH 04/13] cache_req: add support for user by uid
>
> src/responder/common/responder_cache_req.c  | 103 +++++++-
>  src/responder/common/responder_cache_req.h  |  17 +-
>  src/responder/ifp/ifpsrv_cmd.c              |   4 +-
>  src/tests/cmocka/test_responder_cache_req.c | 370 +++++++++++++++++++++++++++-
>  4 files changed, 483 insertions(+), 11 deletions(-)
>
> diff --git a/src/responder/common/responder_cache_req.c b/src/responder/common/responder_cache_req.c
> index b10dfbd783be1c8f4782f0b765644ca1083749a7..76756fcee6652ac5d524e8427dad827a9d3fa1ed 100644
> --- a/src/responder/common/responder_cache_req.c
> +++ b/src/responder/common/responder_cache_req.c
> @@ -33,6 +33,7 @@ struct cache_req_input {
>  /* Provided input. */
>  const char *orig_name;
>
> uint32_t id;
>
> /* Data Provider request type resolved from @type. */
> enum sss_dp_acct_type dp_type;
>
> @@ -52,7 +53,8 @@ struct cache_req_input {
>  struct cache_req_input *
>  cache_req_input_create(TALLOC_CTX *mem_ctx,
>                         enum cache_req_type type,
>                   const char *name)
>                   const char *name,
>                   uint32_t id)
>
> {
>      struct cache_req_input *input;
>
> @@ -77,11 +79,20 @@ cache_req_input_create(TALLOC_CTX *mem_ctx,
>              goto fail;
>          }
>          break;
> case CACHE_REQ_USER_BY_ID:
>    if (id == 0) {
>        DEBUG(SSSDBG_CRIT_FAILURE, "Bug: id cannot be 0!\n");
>        goto fail;
>    }
>    input->id = id;
>    break;

I'm not sure the cache_req should error out here, just return ENOENT
which would be more likely to be handled gracefully.
>  }
>
>  /* Resolve Data Provider request type. */
>  switch (type) {
>  case CACHE_REQ_USER_BY_NAME:
> case CACHE_REQ_USER_BY_ID:
>     input->dp_type = SSS_DP_USER;
>     break;
>
> @@ -138,6 +149,14 @@ cache_req_input_set_domain(struct cache_req_input *input,
>          }
>      break;
>
> case CACHE_REQ_USER_BY_ID:
>    fqn = talloc_asprintf(tmp_ctx, "UID:%d@%s", input->id, domain->name);
>    if (fqn == NULL) {
>        ret = ENOMEM;
>        goto done;
>    }
>    break;
>
> }
>
> input->domain = domain;
>
> @@ -163,6 +182,9 @@ static errno_t cache_req_check_ncache(struct cache_req_input *input,
>          ret = sss_ncache_check_user(ncache, neg_timeout,
>                                      input->domain, input->safe_name);
>          break;
> case CACHE_REQ_USER_BY_ID:
>    ret = sss_ncache_check_uid(ncache, neg_timeout, input->id);
>    break;
> default:
>        ret = EINVAL;
>        DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported cache request type\n");
>
> @@ -188,6 +210,41 @@ static void cache_req_add_to_ncache(struct cache_req_input *input,
>          ret = sss_ncache_set_user(ncache, false, input->domain,
>                                    input->safe_name);
>          break;
> case CACHE_REQ_USER_BY_ID:
>    /* Nothing to do. Those types must be unique among all domains so
>     * the don't contain domain part. Therefore they must be set only
>     * if all domains are search and the entry is not found. */

You must initialize ret here.
>    break;
> default:
>    ret = EINVAL;
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported cache request type\n");
>    break;
> }
> if (ret != EOK) {
>    DEBUG(SSSDBG_MINOR_FAILURE, "Cannot set negcache for [%s] [%d]: %s\n",
>          input->fqn, ret, sss_strerror(ret));
>    /* not fatal */
> }
> return;
>
> +}
>
> +static void cache_req_add_to_ncache_global(struct cache_req_input *input,
>                                       struct sss_nc_ctx *ncache)

When you switch the cache_req structure into the per-domain and per-lookup
type, can you also change the negcache functions to accept per-domain
and per-lookup structures? That's exactly one case where we want to be
careful with using the right data.
> +{
> errno_t ret;
> switch (input->type) {
> case CACHE_REQ_USER_BY_NAME:
> case CACHE_REQ_INITGROUPS:

You must initialize ret here.

[....]

The other patches looked mostly good to me, they are variants on a
similar topic, so I will just highlight one repated pattern:
> struct tevent_req *
> +cache_req_group_by_name_send(TALLOC_CTX *mem_ctx,
>                         struct tevent_context *ev,
>                         struct resp_ctx *rctx,
>                         struct sss_nc_ctx *ncache,
>                         int neg_timeout,
>                         int cache_refresh_percent,
>                         const char *domain,
>                         const char *name)
>
> +{
> struct cache_req_input *input;
> input = cache_req_input_create(mem_ctx, CACHE_REQ_GROUP_BY_NAME, name, 0);
> if (input == NULL) {
>    return NULL;
> }

cache_req_input_create can fail for multiple reasons, I think we should
add a new error code instead of returning NULL. When NULL is returned
from the send function, it usually is treated as ENOMEM by the caller.

So far I read through all the patches. I haven't been able to do any
testing, but I like the direction.

One additional question - are you also planning to add lookups by UPN?
It seems that's the only functionality not implemented by this shared
lookup code.

[Pavel Březina]

New patch set is attached, I hoped it covers all the issues. Some of 
them are implemented as separate patch because either is was too 
complicated to do the change in the middle of pachset or I wanted the 
changes to be more visible.

On 02/17/2015 03:50 PM, Jakub Hrozek wrote:
> > struct tevent_req *
> > +cache_req_group_by_name_send(TALLOC_CTX *mem_ctx,
> >                         struct tevent_context *ev,
> >                         struct resp_ctx *rctx,
> >                         struct sss_nc_ctx *ncache,
> >                         int neg_timeout,
> >                         int cache_refresh_percent,
> >                         const char *domain,
> >                         const char *name)
> >
> > +{
> > struct cache_req_input *input;
> > input = cache_req_input_create(mem_ctx, CACHE_REQ_GROUP_BY_NAME, name, 0);
> > if (input == NULL) {
> >    return NULL;
> > }
>
> cache_req_input_create can fail for multiple reasons, I think we should
> add a new error code instead of returning NULL. When NULL is returned
> from the send function, it usually is treated as ENOMEM by the caller.

I made cache_req_input_create() to fail only on ENOMEM and move check 
id==0 to cache_req_send since it is a valid query unlike name==NULL.
> One additional question - are you also planning to add lookups by UPN?
> It seems that's the only functionality not implemented by this shared
> lookup code.

Thank you for reminding me. The original code predates UPN changes. I 
will create a patch after this huge patch set it accepted. Do you agree?
> In general I like the approach you've taken but I think we can make it
> even better. Add another structure like cache_req_dom_ctx and add it inside
> the cache_req_input structure. Always create cache_req_dom_ctx as a
> talloc child of cache_req_input.
>
> This would not only collapse the above talloc_frees into one, but it
> would also nicely separate the per-domain data from per-lookup data and
> avoid mixing data from different domains.

I played with this a little bit but it actually turned out that it makes 
the code more complicated. Since you need to have access to both input 
and dom_ctx anyway it doesn't really bring anything from above. I don't 
have a patch ready since I abandoned it before working completion. Do 
you want me to try again and finish it?
> I also think dom_name sounds better than safe_name, but meh.

I went with dom_objname since "dom_name" refers to domain name.

[Jakub Hrozek]

On Wed, Feb 25, 2015 at 03:30:17PM +0100, Pavel Březina wrote:
> New patch set is attached, I hoped it covers all the issues. Some of them
> are implemented as separate patch because either is was too complicated to
> do the change in the middle of pachset or I wanted the changes to be more
> visible.
>
> On 02/17/2015 03:50 PM, Jakub Hrozek wrote:
> > > struct tevent_req *
> > > +cache_req_group_by_name_send(TALLOC_CTX *mem_ctx,
> > >                         struct tevent_context *ev,
> > >                         struct resp_ctx *rctx,
> > >                         struct sss_nc_ctx *ncache,
> > >                         int neg_timeout,
> > >                         int cache_refresh_percent,
> > >                         const char *domain,
> > >                         const char *name)
> > >
> > > +{
> > > struct cache_req_input *input;
> > > input = cache_req_input_create(mem_ctx, CACHE_REQ_GROUP_BY_NAME, name, 0);
> > > if (input == NULL) {
> > >    return NULL;
> > > }
> >
> > cache_req_input_create can fail for multiple reasons, I think we should
> > add a new error code instead of returning NULL. When NULL is returned
> > from the send function, it usually is treated as ENOMEM by the caller.
>
> I made cache_req_input_create() to fail only on ENOMEM and move check id==0
> to cache_req_send since it is a valid query unlike name==NULL.

Thanks.
> > One additional question - are you also planning to add lookups by UPN?
> > It seems that's the only functionality not implemented by this shared
> > lookup code.
>
> Thank you for reminding me. The original code predates UPN changes. I will
> create a patch after this huge patch set it accepted. Do you agree?

Sure. We "just" need to implement the UPN lookups before this shared
code is reused by the NSS responder.
> > In general I like the approach you've taken but I think we can make it
> > even better. Add another structure like cache_req_dom_ctx and add it inside
> > the cache_req_input structure. Always create cache_req_dom_ctx as a
> > talloc child of cache_req_input.
> >
> > This would not only collapse the above talloc_frees into one, but it
> > would also nicely separate the per-domain data from per-lookup data and
> > avoid mixing data from different domains.
>
> I played with this a little bit but it actually turned out that it makes the
> code more complicated. Since you need to have access to both input and
> dom_ctx anyway it doesn't really bring anything from above. I don't have a
> patch ready since I abandoned it before working completion. Do you want me
> to try again and finish it?

If it's too complex then no. I was just trying to prevent a bug I saw
recently in the pam responder (I think?) where we would re-use input
data where we should have used per-domain formatted data.
> > I also think dom_name sounds better than safe_name, but meh.
>
> I went with dom_objname since "dom_name" refers to domain name.

The patches now look good to me. I've rebased them on top of the recent
cmocka changes:
    https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/log/?h=review

If you agree with the changes (only trivial changes, since the context
around the cmocka setup function changed), then I'll push the patches.

I'm also waiting for a Coverity check to finish.

[Jakub Hrozek]

On Thu, Mar 12, 2015 at 11:16:41AM +0100, Jakub Hrozek wrote:
> I'm also waiting for a Coverity check to finish.

There are some warnings, mostly the same problem copied around:
Error: COMPILER_WARNING:
sssd-1.12.90/src/responder/common/responder_cache_req.c:232:8: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
#     if (ret == EEXIST) {
#        ^
sssd-1.12.90/src/responder/common/responder_cache_req.c:212:13: note: 'ret' was declared here
#     errno_t ret;
#             ^

Error: COMPILER_WARNING:
sssd-1.12.90/src/responder/common/responder_cache_req.c: scope_hint: In function 'cache_req_get_object'
sssd-1.12.90/src/responder/common/responder_cache_req.c:343:8: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
#     if (ret != EOK) {
#        ^

Error: COMPILER_WARNING:
sssd-1.12.90/src/responder/common/responder_cache_req.c:348:30: warning: 'one_item_only' may be used uninitialized in this function [-Wmaybe-uninitialized]
#     } else if (one_item_only && result->count > 1) {
#                              ^

Error: COMPILER_WARNING:
sssd-1.12.90/src/responder/common/responder_cache_req.c:26: included_from: Included from here.
sssd-1.12.90/src/responder/common/responder_cache_req.c: scope_hint: In function 'cache_req_cache_done'
sssd-1.12.90/src/util/util.h:130:9: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
#         debug_fn(__FILE__, __LINE__, __FUNCTION__, \
#         ^
sssd-1.12.90/src/responder/common/responder_cache_req.c:243:13: note: 'ret' was declared here
#     errno_t ret;
#             ^

I think it's fine to initialize ret to ERR_INTERNAL in that case. We know
that one of the 'case' statements would hit, so this change could be just
another precaution.

[Jakub Hrozek]

On Thu, Mar 12, 2015 at 11:50:21AM +0100, Jakub Hrozek wrote:
> On Thu, Mar 12, 2015 at 11:16:41AM +0100, Jakub Hrozek wrote:
> > I'm also waiting for a Coverity check to finish.
>
> There are some warnings, mostly the same problem copied around:
> Error: COMPILER_WARNING:
> sssd-1.12.90/src/responder/common/responder_cache_req.c:232:8: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
> #     if (ret == EEXIST) {
> #        ^
> sssd-1.12.90/src/responder/common/responder_cache_req.c:212:13: note: 'ret' was declared here
> #     errno_t ret;
> #             ^
>
> Error: COMPILER_WARNING:
> sssd-1.12.90/src/responder/common/responder_cache_req.c: scope_hint: In function 'cache_req_get_object'
> sssd-1.12.90/src/responder/common/responder_cache_req.c:343:8: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
> #     if (ret != EOK) {
> #        ^
>
> Error: COMPILER_WARNING:
> sssd-1.12.90/src/responder/common/responder_cache_req.c:348:30: warning: 'one_item_only' may be used uninitialized in this function [-Wmaybe-uninitialized]
> #     } else if (one_item_only && result->count > 1) {
> #                              ^
>
> Error: COMPILER_WARNING:
> sssd-1.12.90/src/responder/common/responder_cache_req.c:26: included_from: Included from here.
> sssd-1.12.90/src/responder/common/responder_cache_req.c: scope_hint: In function 'cache_req_cache_done'
> sssd-1.12.90/src/util/util.h:130:9: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
> #         debug_fn(__FILE__, __LINE__, __FUNCTION__, \
> #         ^
> sssd-1.12.90/src/responder/common/responder_cache_req.c:243:13: note: 'ret' was declared here
> #     errno_t ret;
> #             ^
>
> I think it's fine to initialize ret to ERR_INTERNAL in that case. We know
> that one of the 'case' statements would hit, so this change could be just
> another precaution.

btw CI passed: http://sssd-ci.duckdns.org/logs/job/10/53/summary.html

[Pavel Březina]

On 03/12/2015 12:30 PM, Jakub Hrozek wrote:
> On Thu, Mar 12, 2015 at 11:50:21AM +0100, Jakub Hrozek wrote:
> > On Thu, Mar 12, 2015 at 11:16:41AM +0100, Jakub Hrozek wrote:
> > > I'm also waiting for a Coverity check to finish.
> >
> > There are some warnings, mostly the same problem copied around:
> > Error: COMPILER_WARNING:
> > sssd-1.12.90/src/responder/common/responder_cache_req.c:232:8: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > #     if (ret == EEXIST) {
> > #        ^
> > sssd-1.12.90/src/responder/common/responder_cache_req.c:212:13: note: 'ret' was declared here
> > #     errno_t ret;
> > #             ^
> >
> > Error: COMPILER_WARNING:
> > sssd-1.12.90/src/responder/common/responder_cache_req.c: scope_hint: In function 'cache_req_get_object'
> > sssd-1.12.90/src/responder/common/responder_cache_req.c:343:8: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > #     if (ret != EOK) {
> > #        ^
> >
> > Error: COMPILER_WARNING:
> > sssd-1.12.90/src/responder/common/responder_cache_req.c:348:30: warning: 'one_item_only' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > #     } else if (one_item_only && result->count > 1) {
> > #                              ^
> >
> > Error: COMPILER_WARNING:
> > sssd-1.12.90/src/responder/common/responder_cache_req.c:26: included_from: Included from here.
> > sssd-1.12.90/src/responder/common/responder_cache_req.c: scope_hint: In function 'cache_req_cache_done'
> > sssd-1.12.90/src/util/util.h:130:9: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > #         debug_fn(__FILE__, __LINE__, __FUNCTION__, \
> > #         ^
> > sssd-1.12.90/src/responder/common/responder_cache_req.c:243:13: note: 'ret' was declared here
> > #     errno_t ret;
> > #             ^
> >
> > I think it's fine to initialize ret to ERR_INTERNAL in that case. We know
> > that one of the 'case' statements would hit, so this change could be just
> > another precaution.
>
> btw CI passed: http://sssd-ci.duckdns.org/logs/job/10/53/summary.html

Thanks.

Since those warning comes from original code that is already present in 
master, it is fixed in a separate patch (#1).

[Lukas Slebodnik]

On (12/03/15 12:34), Pavel Březina wrote:
> On 03/12/2015 12:30 PM, Jakub Hrozek wrote:
> > On Thu, Mar 12, 2015 at 11:50:21AM +0100, Jakub Hrozek wrote:
> > > On Thu, Mar 12, 2015 at 11:16:41AM +0100, Jakub Hrozek wrote:
> > > > I'm also waiting for a Coverity check to finish.
> > >
> > > There are some warnings, mostly the same problem copied around:
> > > Error: COMPILER_WARNING:
> > > sssd-1.12.90/src/responder/common/responder_cache_req.c:232:8: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > > #     if (ret == EEXIST) {
> > > #        ^
> > > sssd-1.12.90/src/responder/common/responder_cache_req.c:212:13: note: 'ret' was declared here
> > > #     errno_t ret;
> > > #             ^
> > >
> > > Error: COMPILER_WARNING:
> > > sssd-1.12.90/src/responder/common/responder_cache_req.c: scope_hint: In function 'cache_req_get_object'
> > > sssd-1.12.90/src/responder/common/responder_cache_req.c:343:8: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > > #     if (ret != EOK) {
> > > #        ^
> > >
> > > Error: COMPILER_WARNING:
> > > sssd-1.12.90/src/responder/common/responder_cache_req.c:348:30: warning: 'one_item_only' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > > #     } else if (one_item_only && result->count > 1) {
> > > #                              ^
> > >
> > > Error: COMPILER_WARNING:
> > > sssd-1.12.90/src/responder/common/responder_cache_req.c:26: included_from: Included from here.
> > > sssd-1.12.90/src/responder/common/responder_cache_req.c: scope_hint: In function 'cache_req_cache_done'
> > > sssd-1.12.90/src/util/util.h:130:9: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > > #         debug_fn(__FILE__, __LINE__, __FUNCTION__, \
> > > #         ^
> > > sssd-1.12.90/src/responder/common/responder_cache_req.c:243:13: note: 'ret' was declared here
> > > #     errno_t ret;
> > > #             ^
> > >
> > > I think it's fine to initialize ret to ERR_INTERNAL in that case. We know
> > > that one of the 'case' statements would hit, so this change could be just
> > > another precaution.
> >
> > btw CI passed: http://sssd-ci.duckdns.org/logs/job/10/53/summary.html
>
> Thanks.
>
> Since those warning comes from original code that is already present in
> master, it is fixed in a separate patch (#1).

I do not agree with separate patch.
Firstly, it is compiler warning and not coverity
Secondly, it is caused by 7th patch
    cache_req: remove default branch from switches

CC       src/responder/common/responder_cache_req.o
src/responder/common/responder_cache_req.c: In function ‘cache_req_get_object’:
src/responder/common/responder_cache_req.c:304:8: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
     if (ret != EOK) {
        ^
src/responder/common/responder_cache_req.c:309:30: error: ‘one_item_only’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
     } else if (one_item_only && result->count > 1) {
                              ^
src/responder/common/responder_cache_req.c: In function ‘cache_req_cache_done’:
src/responder/common/responder_cache_req.c:234:160: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
src/responder/common/responder_cache_req.c:213:13: note: ‘ret’ was declared here
     errno_t ret;
             ^
src/responder/common/responder_cache_req.c: In function ‘cache_req_next_domain’:
src/responder/common/responder_cache_req.c:262:160: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
src/responder/common/responder_cache_req.c:246:13: note: ‘ret’ was declared here
     errno_t ret;
             ^
src/responder/common/responder_cache_req.c:202:8: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
     if (ret == EEXIST) {
        ^
src/responder/common/responder_cache_req.c:185:13: note: ‘ret’ was declared here
     errno_t ret;
             ^
cc1: all warnings being treated as errors

sh$ rpm -q gcc
gcc-5.0.0-0.17.fc22.x86_64

I did not testen on fedora 21.

LS

[Pavel Březina]

On 03/12/2015 01:05 PM, Lukas Slebodnik wrote:
> On (12/03/15 12:34), Pavel Březina wrote:
> > On 03/12/2015 12:30 PM, Jakub Hrozek wrote:
> > > On Thu, Mar 12, 2015 at 11:50:21AM +0100, Jakub Hrozek wrote:
> > > > On Thu, Mar 12, 2015 at 11:16:41AM +0100, Jakub Hrozek wrote:
> > > > > I'm also waiting for a Coverity check to finish.
> > > >
> > > > There are some warnings, mostly the same problem copied around:
> > > > Error: COMPILER_WARNING:
> > > > sssd-1.12.90/src/responder/common/responder_cache_req.c:232:8: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > > > #     if (ret == EEXIST) {
> > > > #        ^
> > > > sssd-1.12.90/src/responder/common/responder_cache_req.c:212:13: note: 'ret' was declared here
> > > > #     errno_t ret;
> > > > #             ^
> > > >
> > > > Error: COMPILER_WARNING:
> > > > sssd-1.12.90/src/responder/common/responder_cache_req.c: scope_hint: In function 'cache_req_get_object'
> > > > sssd-1.12.90/src/responder/common/responder_cache_req.c:343:8: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > > > #     if (ret != EOK) {
> > > > #        ^
> > > >
> > > > Error: COMPILER_WARNING:
> > > > sssd-1.12.90/src/responder/common/responder_cache_req.c:348:30: warning: 'one_item_only' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > > > #     } else if (one_item_only && result->count > 1) {
> > > > #                              ^
> > > >
> > > > Error: COMPILER_WARNING:
> > > > sssd-1.12.90/src/responder/common/responder_cache_req.c:26: included_from: Included from here.
> > > > sssd-1.12.90/src/responder/common/responder_cache_req.c: scope_hint: In function 'cache_req_cache_done'
> > > > sssd-1.12.90/src/util/util.h:130:9: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > > > #         debug_fn(__FILE__, __LINE__, __FUNCTION__, \
> > > > #         ^
> > > > sssd-1.12.90/src/responder/common/responder_cache_req.c:243:13: note: 'ret' was declared here
> > > > #     errno_t ret;
> > > > #             ^
> > > >
> > > > I think it's fine to initialize ret to ERR_INTERNAL in that case. We know
> > > > that one of the 'case' statements would hit, so this change could be just
> > > > another precaution.
> > >
> > > btw CI passed: http://sssd-ci.duckdns.org/logs/job/10/53/summary.html
> >
> > Thanks.
> >
> > Since those warning comes from original code that is already present in
> > master, it is fixed in a separate patch (#1).
>
> I do not agree with separate patch.
> Firstly, it is compiler warning and not coverity
> Secondly, it is caused by 7th patch
>      cache_req: remove default branch from switches
>
> CC       src/responder/common/responder_cache_req.o
> src/responder/common/responder_cache_req.c: In function ‘cache_req_get_object’:
> src/responder/common/responder_cache_req.c:304:8: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>       if (ret != EOK) {
>          ^
> src/responder/common/responder_cache_req.c:309:30: error: ‘one_item_only’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>       } else if (one_item_only && result->count > 1) {
>                                ^
> src/responder/common/responder_cache_req.c: In function ‘cache_req_cache_done’:
> src/responder/common/responder_cache_req.c:234:160: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> src/responder/common/responder_cache_req.c:213:13: note: ‘ret’ was declared here
>       errno_t ret;
>               ^
> src/responder/common/responder_cache_req.c: In function ‘cache_req_next_domain’:
> src/responder/common/responder_cache_req.c:262:160: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> src/responder/common/responder_cache_req.c:246:13: note: ‘ret’ was declared here
>       errno_t ret;
>               ^
> src/responder/common/responder_cache_req.c:202:8: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>       if (ret == EEXIST) {
>          ^
> src/responder/common/responder_cache_req.c:185:13: note: ‘ret’ was declared here
>       errno_t ret;
>               ^
> cc1: all warnings being treated as errors
>
> sh$ rpm -q gcc
> gcc-5.0.0-0.17.fc22.x86_64
>
> I did not testen on fedora 21.
>
> LS
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Ok, new patches attached.

[Jakub Hrozek]

On Thu, Mar 12, 2015 at 02:10:53PM +0100, Pavel Březina wrote:
> Ok, new patches attached.

ACK

CI passes despite some Jenkins issues:
    http://sssd-ci.duckdns.org/logs/job/10/63/summary.html

[Jakub Hrozek]

On Fri, Mar 13, 2015 at 10:34:52AM +0100, Jakub Hrozek wrote:
> On Thu, Mar 12, 2015 at 02:10:53PM +0100, Pavel Březina wrote:
> > Ok, new patches attached.
>
> ACK
>
> CI passes despite some Jenkins issues:
>     http://sssd-ci.duckdns.org/logs/job/10/63/summary.html

* master:
    * 997864d4953a655f6ee4fe27b70fdaa30bd7790e
    * e87b2a6e94c1066b3044fe683825ff5b4f8716c2
    * 282203aa6a64967af029594a41a2cbfe3d5d3787
    * 71965bb18407ff45ada9e47cb6def086e48663c6
    * 4458dbab001a9718de7fd3b39515183330d370c4
    * 641d684ee88c6540a4cf1d74d258614f615699fe
    * 3a5ea81007bd38ce511c37f65cc45d4b6b95ec44
    * 665bc06b1a39c64227de74ecbba3db1c4c104ccf
    * bbc34d5a6e84d6c337bd89a22d33e365eb466226
    * 54ed1b1214dbf9da1f481e8d193c81ce4312516b