On Fri, Sep 26, 2014 at 01:41:39PM +0200, Pavel Březina wrote:
On 09/26/2014 01:38 PM, Pavel Březina wrote:
>On Wed, Sep 24, 2014 at 10:25:22AM +0200, Sumit Bose wrote:
>>Hi,
>>
>>this patches contain the IPA provider part of
>>https://fedorahosted.org/sssd/ticket/2375 as described in the FreeIPA
>>design page
>>http://www.freeipa.org/page/V4/Migrating_existing_environments_to_Trust
>>
>>Patches 0001, 0006 and 0007 contain new sysdb interfaces for views and
>>overrides. 0002 and 0003 refactor some already existing calls.
>>
>>0004 adds the initial support for views and overrides and reads the view
>>name for the client from the FreeIPA server or sets it to 'default' when
>>running in ipa-server-mode. It adds some new options which currently
>>misses man page entries and are not added to the python config API. I
>>did this on purpose for the time being because I wanted to see first if
>>the list is complete or if some options ca be dropped.
>>
>>0005 adds the request to get a specific override from the FreeIPA server
>>and finally in 0008 the overrides are read during a request and saves or
>>applied.
>>
>>This patches must be applied on to of the extdom patch I send yesterday.
>>To add and manage views and override on the server side please have a
>>look at Tomas's patch set on the freeipa-devel list
>>(https://www.redhat.com/archives/freeipa-devel/2014-September/msg00336.html).
>>
>>
>>bye,
>>Sumit
Pavel, Jakub, thank you for the review.
>
>>From dedbb5ae2faa13d396e84ce9911ab1e9c0246836 Mon Sep 17 00:00:00 2001
>>From: Sumit Bose <sbose(a)redhat.com>
>>Date: Tue, 16 Sep 2014 15:18:53 +0200
>>Subject: [PATCH 1/8] sysdb: add sysdb_update_view_name()
>
>ACK
>
>A unit test for the functions would be nice in order to avoid decreasing
>the code coverage of the sysdb module, but I realize we have a deadline.
>We should file a ticket, though.
yes, I feel quite bad about the missing test, but my plan is to add them
after the dealine
>
>It would also be nice to add a comment that it's not possible to have a
>view w/o a name but we are trying to be on the safe side b/c name is not
>MUST as we discussed on IRC with Sumit.
done
>
>>From c09a8553d7371e46b84e980f38d017c7f84c2f87 Mon Sep 17 00:00:00 2001
>>From: Sumit Bose <sbose(a)redhat.com>
>>Date: Tue, 16 Sep 2014 15:22:08 +0200
>>Subject: [PATCH 2/8] Add sdap_deref_search_with_filter_send()
>
>I would prefer to also add a matching _done and _recv functions, even if
>they were wrappers around existing functions. But tevent coding style is
>important to follow, we already had bugs about reusing a single tevent
>callbacks for multiple requests.
done
>
>We can do it post-rebase, though. Just please file a ticket.
>
>
>>From 3097c76ba9498b90df69935d24dc9ce55f602937 Mon Sep 17 00:00:00 2001
>>From: Sumit Bose <sbose(a)redhat.com>
>>Date: Fri, 19 Sep 2014 10:26:01 +0200
>>Subject: [PATCH 3/8] IPA: make IPA ID context available to extdom
>>client code
>
>ACK
>
>...aaand I stopped here.
>
>That was Jakub's part of the review.
>
>Patch 4: IPA: add view support and get view name
>
>> if (ret != EOK) {
>>- DEBUG(SSSDBG_CRIT_FAILURE, "Unable to set SRV lookup
>>plugin "
>>- "[%d]: %s\n", ret,
>>strerror(ret));
>>- goto done;
>>+ if (ret == ENOENT) {
>>+ DEBUG(SSSDBG_CRIT_FAILURE,
>>+ "Cannot find view name in the cache. " \
>>+ "Will do online llokup later\n");
> ^ typo
fixed
>>+ } else {
>>+ DEBUG(SSSDBG_OP_FAILURE, "sysdb_get_view_name
>>failed.\n");
>>+ goto done;
>>+ }
>>+ }
>
>> ctx->ranges_search_bases =
id_ctx->ipa_options->ranges_search_bases;
>>+ ctx->host_search_bases = id_ctx->ipa_options->host_search_bases;
>> ctx->configured_explicit = configured_explicit;
>
>This change seems to be unrelated to this patch. Shouldn't it be a
>separate commit?
no, the view name is an attribute of the host object so we must be able
to search it.
>
>Patch 5: views: add ipa_get_ad_override_send()
>
>Looks good, but can you add some more tracing debug messages there? For
>example what view are we searching for and that an override was found.
done
>
>Patch 6:
>
>>+ const char *obj_attrs[] = { SYSDB_OBJECTCLASS,
>>+ SYSDB_OVERRIDE_DN,
>>+ NULL};
>
>Indentation.
fixed
>
>>+ if (ret != EOK) {
>>+ DEBUG(SSSDBG_CRIT_FAILURE,
>>+ "Missing anchor in overide attributes.\n");
> ^ typo
fixed
>>+ ret = EINVAL;
>>+ goto done;
>>+ }
>
>>+ if (ret != EOK) {
>>+ if (ret == ENOENT) {
>>+ DEBUG(SSSDBG_CRIT_FAILURE, "Object to override does not
>>exists.\n");
>>+ } else {
>>+ DEBUG(SSSDBG_OP_FAILURE, "sysdb_search_entry failed.\n");
>>+ }
>>+ goto done;
>>+ }
>
>Can you use if (ret == ENOENT) else if (ret != EOK) here? I think it is
>simpler.
I prefer it the other way, becasue I think it makes it more clear that
it is an error in all cases and only adds a special messages for the
ENOENT case.
>
>>+ if (obj_override_dn != NULL) {
>>+ if (strcmp(obj_override_dn, override_dn_str) != 0) {
>>+ DEBUG(SSSDBG_CRIT_FAILURE,
>>+ "Existing [%s] and new [%s] overid DN do not
>>match.\n",
> ^ typo
fixed
>>+ obj_override_dn, override_dn_str);
>>+ ret = EINVAL;
>>+ goto done;
>>+ }
>>+
>>+ add_ref = false;
>>+ }
>
>Patch 7: sysdb: sysdb_apply_default_override
>
>Ack.
>
>Patch 8: views: get overrides during user and group lookups
>
>Ack.
BTW I checked out your repo. The patches couldn't be applied to master even
with the extdom patch here on the list.
I send a new version of the extdom patch as well all rebase on master, I
hope it will work now.
New version attached.
bye,
Sumit
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel