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