----- Original Message -----
> On (08/09/14 10:02), Yassir Elley wrote:
> >----- Original Message -----
> >> On Fri, Sep 05, 2014 at 06:49:30PM -0400, Yassir Elley wrote:
> >> >
> >> >
> >> > ----- Original Message -----
> >> > > On Wed, Aug 27, 2014 at 05:24:37PM -0400, Yassir Elley wrote:
> >> > > > Hi,
> >> > > >
> >> >
> >> > Thanks for the review! Revised patches attached.
> >>
> >> Thank you, all my concerns are addresses, but I'm afaid I have some
new
> >> comments:
> >
> >Regards,
> >Yassir.
>
> just few nitpicks from static analysers.
>
> >From f619ff819498b2d9eb3f492d9b3c9fac5cfb7be6 Mon Sep 17 00:00:00 2001
> >From: Yassir Elley <yelley(a)redhat.com>
> >Date: Fri, 5 Sep 2014 18:45:50 -0400
> >Subject: [PATCH 2/2] AD-GPO: processing changes for gpo_map_* options
> >
> >---
> > src/providers/ad/ad_access.c | 3 +-
> > src/providers/ad/ad_access.h | 13 ++
> > src/providers/ad/ad_gpo.c | 442
> > +++++++++++++++++++++++++++++++++++++++++--
> > src/providers/ad/ad_gpo.h | 3 +-
> > src/providers/ad/ad_init.c | 21 ++
> > 5 files changed, 459 insertions(+), 23 deletions(-)
> >
> >diff --git a/src/providers/ad/ad_access.c b/src/providers/ad/ad_access.c
> >index
>
>5b1792223c6e1d9fd6906851e8b32922fa88056d..00b5c9e834c975c538169cb8a12396f6a14fa3fe
> >100644
> >--- a/src/providers/ad/ad_access.c
> >+++ b/src/providers/ad/ad_access.c
> >@@ -402,7 +402,8 @@ ad_sdap_access_done(struct tevent_req *subreq)
> > state->be_ctx->ev,
> > state->domain,
> > state->ctx,
> >- state->pd->user);
> >+ state->pd->user,
> >+ state->pd->service);
> >
> > if (!subreq) {
> > tevent_req_error(req, ENOMEM);
> >diff --git a/src/providers/ad/ad_access.h b/src/providers/ad/ad_access.h
> >index
>
>60ff5db66c9663e736338cdf7694eb75584c3674..34f5d2d1b6d8641536d2e1b78d686e27a0ff1585
> >100644
> >--- a/src/providers/ad/ad_access.h
> >+++ b/src/providers/ad/ad_access.h
> >@@ -34,6 +34,19 @@ struct ad_access_ctx {
> > GPO_ACCESS_CONTROL_ENFORCING
> > } gpo_access_control_mode;
> > int gpo_cache_timeout;
> >+ /* supported GPO map options */
> >+ enum gpo_map_type {
> >+ GPO_MAP_INTERACTIVE = 0,
> >+ GPO_MAP_REMOTE_INTERACTIVE,
> >+ GPO_MAP_NETWORK,
> >+ GPO_MAP_BATCH,
> >+ GPO_MAP_SERVICE,
> >+ GPO_MAP_PERMIT,
> >+ GPO_MAP_DENY,
> >+ GPO_MAP_NUM_OPTS
> >+ } gpo_map_type;
> >+ hash_table_t *gpo_map_options_table;
> >+ enum gpo_map_type gpo_default_right;
> > };
> >
> > void
> >diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
> >index
>
>e0905a2df6fb8a4414b10e16dbdf9bb2258dad7e..8b74eeed7b7e70510e54f26ba80ed463e5eaeed0
> >100644
> >--- a/src/providers/ad/ad_gpo.c
> >+++ b/src/providers/ad/ad_gpo.c
> //snip
> >+
> >+errno_t
> >+ad_gpo_parse_map_option_helper(enum gpo_map_type gpo_map_type,
> >+ hash_key_t key,
> >+ hash_table_t *options_table)
> >+{
> >+ hash_value_t val;
> >+ int hret;
> >+ int ret;
> >+
> >+ hret = hash_lookup(options_table, &key, &val);
> >+ if (hret != HASH_SUCCESS && hret != HASH_ERROR_KEY_NOT_FOUND) {
> >+ DEBUG(SSSDBG_OP_FAILURE, "Error checking hash table:
[%s]\n",
> >+ hash_error_string(hret));
> >+ ret = EINVAL;
> >+ goto done;
> >+ } else if (hret == HASH_SUCCESS) {
> >+ /* handle unexpected case where mapping for key already exists */
> >+ if (val.i == gpo_map_type) {
> >+ /* mapping for key exists for same map type; no error */
> >+ DEBUG(SSSDBG_TRACE_FUNC,
> >+ "PAM service %s maps to %s multiple times\n",
key.str,
> >+ gpo_map_type_string(gpo_map_type));
> >+ ret = EOK;
> >+ } else {
> >+ /* mapping for key exists for different map type; error! */
> >+ DEBUG(SSSDBG_CRIT_FAILURE,
> >+ "PAM service %s maps to both %s and %s\n",
key.str,
> >+ gpo_map_type_string(val.i),
> >gpo_map_type_string(gpo_map_type));
> >+ ret = EINVAL;
> >+ }
> >+ goto done;
> >+ } else {
> >+ /* handle expected case where mapping for key doesn't already
exist
> >*/
> >+ val.type = HASH_VALUE_INT;
> >+ val.i = gpo_map_type;
> >+
> >+ hret = hash_enter(options_table, &key, &val);
> >+ if (hret != HASH_SUCCESS) {
> >+ DEBUG(SSSDBG_OP_FAILURE, "Error checking hash table:
[%s]\n",
> >+ hash_error_string(hret));
> >+ ret = EIO;
> >+ }
> EIO is ignored here.
> missing goto done.
> >+ ret = EOK;
> >+ }
> >+
> >+done:
> >+ return ret;
> >+}
> >+
>
> //snip
>
> >+
> >+ hret = hash_lookup(ctx->gpo_map_options_table, &key, &val);
> >+ if (hret != HASH_SUCCESS && hret != HASH_ERROR_KEY_NOT_FOUND) {
> >+ DEBUG(SSSDBG_OP_FAILURE, "Error checking hash table:
[%s]\n",
> >+ hash_error_string(hret));
> >+ ret = EINVAL;
> >+ goto immediately;
> >+ }
> >+
> >+ /* if service isn't mapped, map it to value of ad_gpo_default_right
> >option */
> >+ if (hret == HASH_ERROR_KEY_NOT_FOUND) {
> >+ DEBUG(SSSDBG_TRACE_FUNC, "using default right\n");
> >+ gpo_map_type = ctx->gpo_default_right;
> >+ } else {
> >+ gpo_map_type = val.i;
> Could you use explicit casting?
> - gpo_map_type = val.i;
> + gpo_map_type = (enum gpo_map_type) val.i;
>
> It will silence warning:
> Error: PW.MIXED_ENUM_TYPE: [#def2]
> sssd-1.12.1/src/providers/ad/ad_gpo.c:1257: mixed_enum_type: enumerated type
> mixed with another type
>
> >+ }
> >+
> >+ DEBUG(SSSDBG_TRACE_FUNC, "service %s maps to %s\n", service,
> >+ gpo_map_type_string(gpo_map_type));
> >+
> >+ if (gpo_map_type == GPO_MAP_PERMIT) {
> >+ ret = EOK;
> >+ tevent_req_done(req);
> >+ tevent_req_post(req, ev);
> >+ goto immediately;
> >+ }
>
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
Revised patches attached.
Regards,
Yassir.