[PATCHES] Add backend part of user views

[Sumit Bose]

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 Březina]

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
> From dedbb5ae2faa13d396e84ce9911ab1e9c0246836 Mon Sep 17 00:00:00 2001
> From: Sumit Bose 
> 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.

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.
> From c09a8553d7371e46b84e980f38d017c7f84c2f87 Mon Sep 17 00:00:00 2001
> From: Sumit Bose 
> 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.

We can do it post-rebase, though. Just please file a ticket.
> From 3097c76ba9498b90df69935d24dc9ce55f602937 Mon Sep 17 00:00:00 2001
> From: Sumit Bose 
> 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
>        } 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?

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.

Patch 6:
> const char *obj_attrs[] = { SYSDB_OBJECTCLASS,
>                          SYSDB_OVERRIDE_DN,
>                               NULL};

Indentation.
>    if (ret != EOK) {
>        DEBUG(SSSDBG_CRIT_FAILURE,
>              "Missing anchor in overide attributes.\n");

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

[Pavel Březina]

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
> > From dedbb5ae2faa13d396e84ce9911ab1e9c0246836 Mon Sep 17 00:00:00 2001
> > From: Sumit Bose 
> > 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.
>
> 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.
> > From c09a8553d7371e46b84e980f38d017c7f84c2f87 Mon Sep 17 00:00:00 2001
> > From: Sumit Bose 
> > 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.
>
> We can do it post-rebase, though. Just please file a ticket.
> > From 3097c76ba9498b90df69935d24dc9ce55f602937 Mon Sep 17 00:00:00 2001
> > From: Sumit Bose 
> > 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
> >        } 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?
>
> 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.
>
> Patch 6:
> > const char *obj_attrs[] = { SYSDB_OBJECTCLASS,
> >                          SYSDB_OVERRIDE_DN,
> >                               NULL};
>
> Indentation.
> >    if (ret != EOK) {
> >        DEBUG(SSSDBG_CRIT_FAILURE,
> >              "Missing anchor in overide attributes.\n");
>                                         ^ typo
> >        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.
> > 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
> >               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.

[Sumit Bose]

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

[Pavel Březina]

On 09/26/2014 04:36 PM, Sumit Bose wrote:
> 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 
> > > > 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

Ack.
> > > > From c09a8553d7371e46b84e980f38d017c7f84c2f87 Mon Sep 17 00:00:00 2001
> > > > From: Sumit Bose 
> > > > 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.
> if (sdap_is_control_supported(sh, LDAP_CONTROL_X_DEREF)) {
>    DEBUG(SSSDBG_TRACE_INTERNAL, "Server supports OpenLDAP deref\n");
>    state->deref_type = SDAP_DEREF_OPENLDAP;
>    subreq = sdap_x_deref_search_send(state, ev, opts, sh, search_base,
>                                      filter, deref_attr, attrs, maps,
>                                      num_maps, timeout);
>    if (!subreq) {
>        DEBUG(SSSDBG_OP_FAILURE, "Cannot start OpenLDAP deref search\n");
>        goto fail;
>    }
> } else {
>    DEBUG(SSSDBG_OP_FAILURE,
>          "Server does not support any known deref method!\n");
>    goto fail;
> }

Would it be possible to use sdap_has_deref_support() instead of manually 
testing the support for OpenLDAP deref or you can't use ASQ for some 
reason?
> 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 
> > > > Date: Fri, 19 Sep 2014 10:26:01 +0200
> > > > Subject: [PATCH 3/8] IPA: make IPA ID context available to extdom
> > > > client code
> > >
> > > ACK

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

Ok. Typo is still there, I'm sending a trivial patch. Feel free to 
squash it.

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

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

Ok.
> > > > 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;
> }
>    case SYSDB_MEMBER_GROUP:
>        ret = ldb_msg_add_string(msg, SYSDB_OBJECTCLASS,
>                                 SYSDB_OVERRIDE_USER_CLASS);
>        break;

Shouldn't it say SYSDB_OVERRIDE_GROUP_CLASS?
> > > Patch 7: sysdb: sysdb_apply_default_override
> > >
> > > Ack.

Ack stays, but can you clarify what is the intended use case of this 
function in the commit message please?
> > > Patch 8: views: get overrides during user and group lookups
> > >
> > > Ack.
> if (override_attrs != NULL) {
>     ret = sysdb_apply_default_override(state->user_dom->sysdb,
>                                        override_attrs,
>                                        state->obj_msg->dn);
>     if (ret != EOK) {
>         DEBUG(SSSDBG_OP_FAILURE, "sysdb_apply_default_override failed.\n");
>         goto fail;
>     }
> }

I thought the sysdb_apply_default_override is used to override original 
object in sysdb with information from the default view. This looks like 
it is overwritten with data from whatever view we are using.
> > 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

I get this warning:
/home/pbrezina/workspace/sssd/src/providers/ipa/ipa_views.c: In function 
'ipa_get_ad_override_done':
/home/pbrezina/workspace/sssd/src/providers/ipa/ipa_views.c:207:9: 
error: format '%d' expects argument of type 'int', but argument 6 has 
type 'size_t' [-Werror=format]

[Pavel Březina]

On 09/29/2014 11:52 AM, Pavel Březina wrote:
> On 09/26/2014 04:36 PM, Sumit Bose wrote:
> > 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 
> > > > > 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
>
> Ack.
> > > > > From c09a8553d7371e46b84e980f38d017c7f84c2f87 Mon Sep 17 00:00:00 2001
> > > > > From: Sumit Bose 
> > > > > 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.
> > if (sdap_is_control_supported(sh, LDAP_CONTROL_X_DEREF)) {
> >    DEBUG(SSSDBG_TRACE_INTERNAL, "Server supports OpenLDAP
> >
> > deref\n");
> >    state->deref_type = SDAP_DEREF_OPENLDAP;
> >    subreq = sdap_x_deref_search_send(state, ev, opts, sh,
> >
> > search_base,
> >                                      filter, deref_attr, attrs,
> >
> > maps,
> >                                      num_maps, timeout);
> >    if (!subreq) {
> >        DEBUG(SSSDBG_OP_FAILURE, "Cannot start OpenLDAP deref
> >
> > search\n");
> >        goto fail;
> >    }
> > } else {
> >    DEBUG(SSSDBG_OP_FAILURE,
> >          "Server does not support any known deref method!\n");
> >    goto fail;
> > }
>
> Would it be possible to use sdap_has_deref_support() instead of manually
> testing the support for OpenLDAP deref or you can't use ASQ for some
> reason?
> > 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 
> > > > > Date: Fri, 19 Sep 2014 10:26:01 +0200
> > > > > Subject: [PATCH 3/8] IPA: make IPA ID context available to extdom
> > > > > client code
> > > >
> > > > ACK
>
> Still 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.
>
> Ok. Typo is still there, I'm sending a trivial patch. Feel free to
> squash it.
>
> Ack.
> > > > 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
>
> Ack.
> > > > 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.
>
> Ok.
> > > > > 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;
> > }
> >    case SYSDB_MEMBER_GROUP:
> >        ret = ldb_msg_add_string(msg, SYSDB_OBJECTCLASS,
> >                                 SYSDB_OVERRIDE_USER_CLASS);
> >        break;
>
> Shouldn't it say SYSDB_OVERRIDE_GROUP_CLASS?
> > > > Patch 7: sysdb: sysdb_apply_default_override
> > > >
> > > > Ack.
>
> Ack stays, but can you clarify what is the intended use case of this
> function in the commit message please?
> > > > Patch 8: views: get overrides during user and group lookups
> > > >
> > > > Ack.
> > if (override_attrs != NULL) {
> >     ret = sysdb_apply_default_override(state->user_dom->sysdb,
> >                                        override_attrs,
> >                                        state->obj_msg->dn);
> >     if (ret != EOK) {
> >         DEBUG(SSSDBG_OP_FAILURE, "sysdb_apply_default_override
> >
> > failed.\n");
> >             goto fail;
> >         }
> >     }
>
> I thought the sysdb_apply_default_override is used to override original
> object in sysdb with information from the default view. This looks like
> it is overwritten with data from whatever view we are using.
> > > 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
>
> I get this warning:
> /home/pbrezina/workspace/sssd/src/providers/ipa/ipa_views.c: In function
> 'ipa_get_ad_override_done':
> /home/pbrezina/workspace/sssd/src/providers/ipa/ipa_views.c:207:9:
> error: format '%d' expects argument of type 'int', but argument 6 has
> type 'size_t' [-Werror=format]

I'm sending patches for the typo and warning to speed things up.

[Sumit Bose]

On Mon, Sep 29, 2014 at 11:52:52AM +0200, Pavel Březina wrote:
> On 09/26/2014 04:36 PM, Sumit Bose wrote:
> > 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 
> > > > > 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
>
> Ack.
> > > > > From c09a8553d7371e46b84e980f38d017c7f84c2f87 Mon Sep 17 00:00:00 2001
> > > > > From: Sumit Bose 
> > > > > 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.
> > if (sdap_is_control_supported(sh, LDAP_CONTROL_X_DEREF)) {
> >    DEBUG(SSSDBG_TRACE_INTERNAL, "Server supports OpenLDAP deref\n");
> >    state->deref_type = SDAP_DEREF_OPENLDAP;
> >    subreq = sdap_x_deref_search_send(state, ev, opts, sh, search_base,
> >                                      filter, deref_attr, attrs, maps,
> >                                      num_maps, timeout);
> >    if (!subreq) {
> >        DEBUG(SSSDBG_OP_FAILURE, "Cannot start OpenLDAP deref search\n");
> >        goto fail;
> >    }
> > } else {
> >    DEBUG(SSSDBG_OP_FAILURE,
> >          "Server does not support any known deref method!\n");
> >    goto fail;
> > }
>
> Would it be possible to use sdap_has_deref_support() instead of manually
> testing the support for OpenLDAP deref or you can't use ASQ for some reason?

no, afaik ASQ can only do base searches and follow the references in the
given DN, so adding a filter won't work.
> > 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 
> > > > > Date: Fri, 19 Sep 2014 10:26:01 +0200
> > > > > Subject: [PATCH 3/8] IPA: make IPA ID context available to extdom
> > > > > client code
> > > >
> > > > ACK
>
> Still 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.
>
> Ok. Typo is still there, I'm sending a trivial patch. Feel free to squash
> it.

squashed in.
> Ack.
> > > > 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
>
> Ack.
> > > > 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.
>
> Ok.
> > > > > 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;
> > }
> >    case SYSDB_MEMBER_GROUP:
> >        ret = ldb_msg_add_string(msg, SYSDB_OBJECTCLASS,
> >                                 SYSDB_OVERRIDE_USER_CLASS);
> >        break;
>
> Shouldn't it say SYSDB_OVERRIDE_GROUP_CLASS?

D'oh, fixed.
> > > > Patch 7: sysdb: sysdb_apply_default_override
> > > >
> > > > Ack.
>
> Ack stays, but can you clarify what is the intended use case of this
> function in the commit message please?

Comment added.
> > > > Patch 8: views: get overrides during user and group lookups
> > > >
> > > > Ack.
> > if (override_attrs != NULL) {
> >        ret = sysdb_apply_default_override(state->user_dom->sysdb,
> >                                           override_attrs,
> >                                           state->obj_msg->dn);
> >        if (ret != EOK) {
> >            DEBUG(SSSDBG_OP_FAILURE, "sysdb_apply_default_override failed.\n");
> >            goto fail;
> >        }
> >    }
>
> I thought the sysdb_apply_default_override is used to override original
> object in sysdb with information from the default view. This looks like it
> is overwritten with data from whatever view we are using.

This code path is in ipa-server-mode where the default view is
hardcoded/fixed by definition. I added a comment to make this clear.
> > > 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
>
> I get this warning:
> /home/pbrezina/workspace/sssd/src/providers/ipa/ipa_views.c: In function
> 'ipa_get_ad_override_done':
> /home/pbrezina/workspace/sssd/src/providers/ipa/ipa_views.c:207:9: error:
> format '%d' expects argument of type 'int', but argument 6 has type 'size_t'
> [-Werror=format]

Your fix is squashed in.

Thank you for the review. New version attached.

bye,
Sumit
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Březina]

On 09/29/2014 01:01 PM, Sumit Bose wrote:
> On Mon, Sep 29, 2014 at 11:52:52AM +0200, Pavel Březina wrote:
> > On 09/26/2014 04:36 PM, Sumit Bose wrote:
> > > 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 
> > > > > > 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
> >
> > Ack.
> > > > > > From c09a8553d7371e46b84e980f38d017c7f84c2f87 Mon Sep 17 00:00:00 2001
> > > > > > From: Sumit Bose 
> > > > > > 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.
> > > if (sdap_is_control_supported(sh, LDAP_CONTROL_X_DEREF)) {
> > >    DEBUG(SSSDBG_TRACE_INTERNAL, "Server supports OpenLDAP deref\n");
> > >    state->deref_type = SDAP_DEREF_OPENLDAP;
> > >    subreq = sdap_x_deref_search_send(state, ev, opts, sh, search_base,
> > >                                      filter, deref_attr, attrs, maps,
> > >                                      num_maps, timeout);
> > >    if (!subreq) {
> > >        DEBUG(SSSDBG_OP_FAILURE, "Cannot start OpenLDAP deref search\n");
> > >        goto fail;
> > >    }
> > > } else {
> > >    DEBUG(SSSDBG_OP_FAILURE,
> > >          "Server does not support any known deref method!\n");
> > >    goto fail;
> > > }
> >
> > Would it be possible to use sdap_has_deref_support() instead of manually
> > testing the support for OpenLDAP deref or you can't use ASQ for some reason?
>
> no, afaik ASQ can only do base searches and follow the references in the
> given DN, so adding a filter won't work.
> > > 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 
> > > > > > Date: Fri, 19 Sep 2014 10:26:01 +0200
> > > > > > Subject: [PATCH 3/8] IPA: make IPA ID context available to extdom
> > > > > > client code
> > > > >
> > > > > ACK
> >
> > Still 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.
> >
> > Ok. Typo is still there, I'm sending a trivial patch. Feel free to squash
> > it.
>
> squashed in.
> > Ack.
> > > > > 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
> >
> > Ack.
> > > > > 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.
> >
> > Ok.
> > > > > > 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;
> > > }
> > >    case SYSDB_MEMBER_GROUP:
> > >        ret = ldb_msg_add_string(msg, SYSDB_OBJECTCLASS,
> > >                                 SYSDB_OVERRIDE_USER_CLASS);
> > >        break;
> >
> > Shouldn't it say SYSDB_OVERRIDE_GROUP_CLASS?
>
> D'oh, fixed.
> > > > > Patch 7: sysdb: sysdb_apply_default_override
> > > > >
> > > > > Ack.
> >
> > Ack stays, but can you clarify what is the intended use case of this
> > function in the commit message please?
>
> Comment added.
> > > > > Patch 8: views: get overrides during user and group lookups
> > > > >
> > > > > Ack.
> > > if (override_attrs != NULL) {
> > >     ret = sysdb_apply_default_override(state->user_dom->sysdb,
> > >                                        override_attrs,
> > >                                        state->obj_msg->dn);
> > >     if (ret != EOK) {
> > >         DEBUG(SSSDBG_OP_FAILURE, "sysdb_apply_default_override failed.\n");
> > >         goto fail;
> > >     }
> > > }
> >
> > I thought the sysdb_apply_default_override is used to override original
> > object in sysdb with information from the default view. This looks like it
> > is overwritten with data from whatever view we are using.
>
> This code path is in ipa-server-mode where the default view is
> hardcoded/fixed by definition. I added a comment to make this clear.
> > > > 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
> >
> > I get this warning:
> > /home/pbrezina/workspace/sssd/src/providers/ipa/ipa_views.c: In function
> > 'ipa_get_ad_override_done':
> > /home/pbrezina/workspace/sssd/src/providers/ipa/ipa_views.c:207:9: error:
> > format '%d' expects argument of type 'int', but argument 6 has type 'size_t'
> > [-Werror=format]
>
> Your fix is squashed in.
>
> Thank you for the review. New version attached.
>
> bye,
> Sumit

Hi,
I'm sending two more trivial debug patches to squash in. Otherwise its 
an code wise ack. I'm running some tests now.

[Sumit Bose]

On Mon, Sep 29, 2014 at 02:52:10PM +0200, Pavel Březina wrote:
> On 09/29/2014 01:01 PM, Sumit Bose wrote:
> > On Mon, Sep 29, 2014 at 11:52:52AM +0200, Pavel Březina wrote:
> > > On 09/26/2014 04:36 PM, Sumit Bose wrote:
> > > > 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 
> > > > > > >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
> > >
> > > Ack.
> > > > > > >From c09a8553d7371e46b84e980f38d017c7f84c2f87 Mon Sep 17 00:00:00 2001
> > > > > > >From: Sumit Bose 
> > > > > > >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.
> > > > if (sdap_is_control_supported(sh, LDAP_CONTROL_X_DEREF)) {
> > > >    DEBUG(SSSDBG_TRACE_INTERNAL, "Server supports OpenLDAP deref\n");
> > > >    state->deref_type = SDAP_DEREF_OPENLDAP;
> > > >    subreq = sdap_x_deref_search_send(state, ev, opts, sh, search_base,
> > > >                                      filter, deref_attr, attrs, maps,
> > > >                                      num_maps, timeout);
> > > >    if (!subreq) {
> > > >        DEBUG(SSSDBG_OP_FAILURE, "Cannot start OpenLDAP deref search\n");
> > > >        goto fail;
> > > >    }
> > > > } else {
> > > >    DEBUG(SSSDBG_OP_FAILURE,
> > > >          "Server does not support any known deref method!\n");
> > > >    goto fail;
> > > > }
> > >
> > > Would it be possible to use sdap_has_deref_support() instead of manually
> > > testing the support for OpenLDAP deref or you can't use ASQ for some reason?
> >
> > no, afaik ASQ can only do base searches and follow the references in the
> > given DN, so adding a filter won't work.
> > > > 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 
> > > > > > >Date: Fri, 19 Sep 2014 10:26:01 +0200
> > > > > > >Subject: [PATCH 3/8] IPA: make IPA ID context available to extdom
> > > > > > >client code
> > > > > >
> > > > > > ACK
> > >
> > > Still 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.
> > >
> > > Ok. Typo is still there, I'm sending a trivial patch. Feel free to squash
> > > it.
> >
> > squashed in.
> > > Ack.
> > > > > > 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
> > >
> > > Ack.
> > > > > > 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.
> > >
> > > Ok.
> > > > > > >+    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;
> > > > }
> > > >    case SYSDB_MEMBER_GROUP:
> > > >        ret = ldb_msg_add_string(msg, SYSDB_OBJECTCLASS,
> > > >                                 SYSDB_OVERRIDE_USER_CLASS);
> > > >        break;
> > >
> > > Shouldn't it say SYSDB_OVERRIDE_GROUP_CLASS?
> >
> > D'oh, fixed.
> > > > > > Patch 7: sysdb: sysdb_apply_default_override
> > > > > >
> > > > > > Ack.
> > >
> > > Ack stays, but can you clarify what is the intended use case of this
> > > function in the commit message please?
> >
> > Comment added.
> > > > > > Patch 8: views: get overrides during user and group lookups
> > > > > >
> > > > > > Ack.
> > > > if (override_attrs != NULL) {
> > > >        ret = sysdb_apply_default_override(state->user_dom->sysdb,
> > > >                                           override_attrs,
> > > >                                           state->obj_msg->dn);
> > > >        if (ret != EOK) {
> > > >            DEBUG(SSSDBG_OP_FAILURE, "sysdb_apply_default_override failed.\n");
> > > >            goto fail;
> > > >        }
> > > >    }
> > >
> > > I thought the sysdb_apply_default_override is used to override original
> > > object in sysdb with information from the default view. This looks like it
> > > is overwritten with data from whatever view we are using.
> >
> > This code path is in ipa-server-mode where the default view is
> > hardcoded/fixed by definition. I added a comment to make this clear.
> > > > > 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
> > >
> > > I get this warning:
> > > /home/pbrezina/workspace/sssd/src/providers/ipa/ipa_views.c: In function
> > > 'ipa_get_ad_override_done':
> > > /home/pbrezina/workspace/sssd/src/providers/ipa/ipa_views.c:207:9: error:
> > > format '%d' expects argument of type 'int', but argument 6 has type 'size_t'
> > > [-Werror=format]
> >
> > Your fix is squashed in.
> >
> > Thank you for the review. New version attached.
> >
> > bye,
> > Sumit
>
> Hi,
> I'm sending two more trivial debug patches to squash in. Otherwise its an
> code wise ack. I'm running some tests now.

Thank you. Jakub, in case there are no other issues, can you add them
when committing the patches, or shall I send a new round?

bye,
Sumit

[Jakub Hrozek]

On Mon, Sep 29, 2014 at 03:00:44PM +0200, Sumit Bose wrote:
> > Hi,
> > I'm sending two more trivial debug patches to squash in. Otherwise its an
> > code wise ack. I'm running some tests now.
>
> Thank you. Jakub, in case there are no other issues, can you add them
> when committing the patches, or shall I send a new round?
>
> bye,
> Sumit

I will run the patches through the IPA CI engine with Tomas' help. If
there are no regressions, I will squash in the patches and push, if
there are, we would do another round.

[Sumit Bose]

On Mon, Sep 29, 2014 at 07:38:54PM +0200, Jakub Hrozek wrote:
> On Mon, Sep 29, 2014 at 03:00:44PM +0200, Sumit Bose wrote:
> > > Hi,
> > > I'm sending two more trivial debug patches to squash in. Otherwise its an
> > > code wise ack. I'm running some tests now.
> >
> > Thank you. Jakub, in case there are no other issues, can you add them
> > when committing the patches, or shall I send a new round?
> >
> > bye,
> > Sumit
>
> I will run the patches through the IPA CI engine with Tomas' help. If
> there are no regressions, I will squash in the patches and push, if
> there are, we would do another round.

Hi,

this new series of patches contain fixes for various issues found by
Pavel, Jakub and Alexander. There is an additional 9th patch which
contains support for searching in the override values, e.g. if the user
name is overridden we have to first search in the override data and the
continue with the original object. See commit message for details.

bye,
Sumit
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Sumit Bose]

On Wed, Oct 01, 2014 at 08:14:21PM +0200, Sumit Bose wrote:
> On Mon, Sep 29, 2014 at 07:38:54PM +0200, Jakub Hrozek wrote:
> > On Mon, Sep 29, 2014 at 03:00:44PM +0200, Sumit Bose wrote:
> > > > Hi,
> > > > I'm sending two more trivial debug patches to squash in. Otherwise its an
> > > > code wise ack. I'm running some tests now.
> > >
> > > Thank you. Jakub, in case there are no other issues, can you add them
> > > when committing the patches, or shall I send a new round?
> > >
> > > bye,
> > > Sumit
> >
> > I will run the patches through the IPA CI engine with Tomas' help. If
> > there are no regressions, I will squash in the patches and push, if
> > there are, we would do another round.
>
> Hi,
>
> this new series of patches contain fixes for various issues found by
> Pavel, Jakub and Alexander. There is an additional 9th patch which
> contains support for searching in the override values, e.g. if the user
> name is overridden we have to first search in the override data and the
> continue with the original object. See commit message for details.

Please find attached the latest version of the patches with even more
fixes for issue found by Alexander, Jakub and Pavel. I tired to keep the
original structure of the patches but a new patch sneaked in which adds
some new sysdb interface which allow to add attribute values with
detection of duplicates.

bye,
Sumit
> bye,
> Sumit
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Sumit Bose]

On Tue, Oct 14, 2014 at 11:16:28AM +0200, Sumit Bose wrote:
> On Wed, Oct 01, 2014 at 08:14:21PM +0200, Sumit Bose wrote:
> > On Mon, Sep 29, 2014 at 07:38:54PM +0200, Jakub Hrozek wrote:
> > > On Mon, Sep 29, 2014 at 03:00:44PM +0200, Sumit Bose wrote:
> > > > > Hi,
> > > > > I'm sending two more trivial debug patches to squash in. Otherwise its an
> > > > > code wise ack. I'm running some tests now.
> > > >
> > > > Thank you. Jakub, in case there are no other issues, can you add them
> > > > when committing the patches, or shall I send a new round?
> > > >
> > > > bye,
> > > > Sumit
> > >
> > > I will run the patches through the IPA CI engine with Tomas' help. If
> > > there are no regressions, I will squash in the patches and push, if
> > > there are, we would do another round.
> >
> > Hi,
> >
> > this new series of patches contain fixes for various issues found by
> > Pavel, Jakub and Alexander. There is an additional 9th patch which
> > contains support for searching in the override values, e.g. if the user
> > name is overridden we have to first search in the override data and the
> > continue with the original object. See commit message for details.
>
> Please find attached the latest version of the patches with even more
> fixes for issue found by Alexander, Jakub and Pavel. I tired to keep the
> original structure of the patches but a new patch sneaked in which adds
> some new sysdb interface which allow to add attribute values with
> detection of duplicates.
>
> bye,
> Sumit

I forgot the say that the patches now depend on the two patches I send
in the "[PATCHES] nss: add SSS_NSS_GETORIGBYNAME request" thread.

bye,
Sumit
> > bye,
> > Sumit
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Sumit Bose]

On Tue, Oct 14, 2014 at 11:24:04AM +0200, Sumit Bose wrote:
> On Tue, Oct 14, 2014 at 11:16:28AM +0200, Sumit Bose wrote:
> > On Wed, Oct 01, 2014 at 08:14:21PM +0200, Sumit Bose wrote:
> > > On Mon, Sep 29, 2014 at 07:38:54PM +0200, Jakub Hrozek wrote:
> > > > On Mon, Sep 29, 2014 at 03:00:44PM +0200, Sumit Bose wrote:
> > > > > > Hi,
> > > > > > I'm sending two more trivial debug patches to squash in. Otherwise its an
> > > > > > code wise ack. I'm running some tests now.
> > > > >
> > > > > Thank you. Jakub, in case there are no other issues, can you add them
> > > > > when committing the patches, or shall I send a new round?
> > > > >
> > > > > bye,
> > > > > Sumit
> > > >
> > > > I will run the patches through the IPA CI engine with Tomas' help. If
> > > > there are no regressions, I will squash in the patches and push, if
> > > > there are, we would do another round.
> > >
> > > Hi,
> > >
> > > this new series of patches contain fixes for various issues found by
> > > Pavel, Jakub and Alexander. There is an additional 9th patch which
> > > contains support for searching in the override values, e.g. if the user
> > > name is overridden we have to first search in the override data and the
> > > continue with the original object. See commit message for details.
> >
> > Please find attached the latest version of the patches with even more
> > fixes for issue found by Alexander, Jakub and Pavel. I tired to keep the
> > original structure of the patches but a new patch sneaked in which adds
> > some new sysdb interface which allow to add attribute values with
> > detection of duplicates.
> >
> > bye,
> > Sumit
>
> I forgot the say that the patches now depend on the two patches I send
> in the "[PATCHES] nss: add SSS_NSS_GETORIGBYNAME request" thread.

This new version fixes some minor issues found by Coverity.

bye,
Sumit
> bye,
> Sumit
> > > bye,
> > > Sumit
> > > > sssd-devel mailing list
> > > > [e-mail]
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Březina]

On 10/14/2014 05:12 PM, Sumit Bose wrote:
> On Tue, Oct 14, 2014 at 11:24:04AM +0200, Sumit Bose wrote:
> > On Tue, Oct 14, 2014 at 11:16:28AM +0200, Sumit Bose wrote:
> > > On Wed, Oct 01, 2014 at 08:14:21PM +0200, Sumit Bose wrote:
> > > > On Mon, Sep 29, 2014 at 07:38:54PM +0200, Jakub Hrozek wrote:
> > > > > On Mon, Sep 29, 2014 at 03:00:44PM +0200, Sumit Bose wrote:
> > > > > > > Hi,
> > > > > > > I'm sending two more trivial debug patches to squash in. Otherwise its an
> > > > > > > code wise ack. I'm running some tests now.
> > > > > > >
> > > > > >
> > > > > > Thank you. Jakub, in case there are no other issues, can you add them
> > > > > > when committing the patches, or shall I send a new round?
> > > > > >
> > > > > > bye,
> > > > > > Sumit
> > > > >
> > > > > I will run the patches through the IPA CI engine with Tomas' help. If
> > > > > there are no regressions, I will squash in the patches and push, if
> > > > > there are, we would do another round.
> > > >
> > > > Hi,
> > > >
> > > > this new series of patches contain fixes for various issues found by
> > > > Pavel, Jakub and Alexander. There is an additional 9th patch which
> > > > contains support for searching in the override values, e.g. if the user
> > > > name is overridden we have to first search in the override data and the
> > > > continue with the original object. See commit message for details.
> > >
> > > Please find attached the latest version of the patches with even more
> > > fixes for issue found by Alexander, Jakub and Pavel. I tired to keep the
> > > original structure of the patches but a new patch sneaked in which adds
> > > some new sysdb interface which allow to add attribute values with
> > > detection of duplicates.
> > >
> > > bye,
> > > Sumit
> >
> > I forgot the say that the patches now depend on the two patches I send
> > in the "[PATCHES] nss: add SSS_NSS_GETORIGBYNAME request" thread.
>
> This new version fixes some minor issues found by Coverity.

Hi,

Patch 1: sysdb: add sysdb_update_view_name()
> static errno_t sysdb_get_view_name_ex(TALLOC_CTX *mem_ctx,
>                                       struct sysdb_ctx *sysdb,
>                                       char **_view_name,
>                                       bool *view_container_exists)
> {

...
> *_view_name = talloc_strdup(mem_ctx, tmp_str);

I think it is possible to just steal the value since ldb uses talloc. 
But if you want to strdup the string you should test for NULL.

Do we really need view_container_exists? I think it is not possible to 
have a view without a name and I would rather report an error in this 
case since it basically means that db is corrupted.
> ret = EOK;
>
> done:
>     talloc_free(tmp_ctx);
>     return ret;
> }
> errno_t sysdb_update_view_name(struct sysdb_ctx *sysdb,
>                                const char *view_name)
> {
>     errno_t ret;
>     TALLOC_CTX *tmp_ctx;
>     char *tmp_str;
>     bool view_container_exists = false;
>     bool add_view_name = false;
>     struct ldb_message *msg;

I suppose this function is not yet completed? add_view_name is basically 
not used since it is always true.
> if (ret == EOK) {
>     if (strcmp(tmp_str, view_name) == 0) {
>         /* view name already known, mothing to do */

^ *typo*
>         DEBUG(SSSDBG_TRACE_ALL, "View name already in place.\n");
>         ret = EOK;
>         goto done;

Patch 2: Add sdap_deref_search_with_filter_send()

ACK

Patch 3: IPA: make IPA ID context available to extdom client code

ACK

Patch 4: IPA: add view support and get view name

ACK

Do you think the list of options is now complete? The new options are 
still missing in man page and config api.

Patch 5: views: add ipa_get_ad_override_send()

ACK

Patch 6: sysdb: add sysdb_store_override

ACK

Patch 7: sysdb: sysdb_apply_default_override

ACK

Patch 8:  sysdb: add sysdb_attrs_add_val_safe() and 
sysdb_attrs_add_string_safe()

ACK

Patch 9: views: get overrides during user and group lookups

ACK

Patch 10: views: search overrides for user and group requests

ACK

I've performed some smoked tests and I'm using sssd for some time while 
working on the responder part. I think it is good to go besides the 
minor issues.

[Lukas Slebodnik]

On (15/10/14 15:37), Pavel Březina wrote:
> On 10/14/2014 05:12 PM, Sumit Bose wrote:
> > On Tue, Oct 14, 2014 at 11:24:04AM +0200, Sumit Bose wrote:
> > > On Tue, Oct 14, 2014 at 11:16:28AM +0200, Sumit Bose wrote:
> > > > On Wed, Oct 01, 2014 at 08:14:21PM +0200, Sumit Bose wrote:
> > > > > On Mon, Sep 29, 2014 at 07:38:54PM +0200, Jakub Hrozek wrote:
> > > > > > On Mon, Sep 29, 2014 at 03:00:44PM +0200, Sumit Bose wrote:
> > > > > > >>Hi,
> > > > > > >>I'm sending two more trivial debug patches to squash in. Otherwise its an
> > > > > > >>code wise ack. I'm running some tests now.
> > > > > > >>
> > > > > > >
> > > > > > >Thank you. Jakub, in case there are no other issues, can you add them
> > > > > > >when committing the patches, or shall I send a new round?
> > > > > > >
> > > > > > >bye,
> > > > > > >Sumit
> > > > > >
> > > > > > I will run the patches through the IPA CI engine with Tomas' help. If
> > > > > > there are no regressions, I will squash in the patches and push, if
> > > > > > there are, we would do another round.
> > > > >
> > > > > Hi,
> > > > >
> > > > > this new series of patches contain fixes for various issues found by
> > > > > Pavel, Jakub and Alexander. There is an additional 9th patch which
> > > > > contains support for searching in the override values, e.g. if the user
> > > > > name is overridden we have to first search in the override data and the
> > > > > continue with the original object. See commit message for details.
> > > >
> > > > Please find attached the latest version of the patches with even more
> > > > fixes for issue found by Alexander, Jakub and Pavel. I tired to keep the
> > > > original structure of the patches but a new patch sneaked in which adds
> > > > some new sysdb interface which allow to add attribute values with
> > > > detection of duplicates.
> > > >
> > > > bye,
> > > > Sumit
> > >
> > > I forgot the say that the patches now depend on the two patches I send
> > > in the "[PATCHES] nss: add SSS_NSS_GETORIGBYNAME request" thread.
> >
> > This new version fixes some minor issues found by Coverity.
>
> Hi,
>
> Patch 1: sysdb: add sysdb_update_view_name()
> > static errno_t sysdb_get_view_name_ex(TALLOC_CTX *mem_ctx,
> >                                      struct sysdb_ctx *sysdb,
> >                                      char **_view_name,
> >                                      bool *view_container_exists)
> > {
>
> ...
> > *_view_name = talloc_strdup(mem_ctx, tmp_str);
>
> I think it is possible to just steal the value since ldb uses talloc. But if
> you want to strdup the string you should test for NULL.
>
> Do we really need view_container_exists? I think it is not possible to have a
> view without a name and I would rather report an error in this case since it
> basically means that db is corrupted.
> > ret = EOK;
> >
> > done:
> >    talloc_free(tmp_ctx);
> >    return ret;
> > }
> > errno_t sysdb_update_view_name(struct sysdb_ctx *sysdb,
> >                               const char *view_name)
> > {
> >    errno_t ret;
> >    TALLOC_CTX *tmp_ctx;
> >    char *tmp_str;
> >    bool view_container_exists = false;
> >    bool add_view_name = false;
> >    struct ldb_message *msg;
>
> I suppose this function is not yet completed? add_view_name is basically not
> used since it is always true.
> > if (ret == EOK) {
> >        if (strcmp(tmp_str, view_name) == 0) {
> >            /* view name already known, mothing to do */
>                                      ^ *typo*
>
> >        DEBUG(SSSDBG_TRACE_ALL, "View name already in place.\n");
> >        ret = EOK;
> >        goto done;
>
> Patch 2: Add sdap_deref_search_with_filter_send()
>
> ACK
>
> Patch 3: IPA: make IPA ID context available to extdom client code
>
> ACK
>
> Patch 4: IPA: add view support and get view name
>
> ACK
>
> Do you think the list of options is now complete? The new options are still
> missing in man page and config api.
>
> Patch 5: views: add ipa_get_ad_override_send()
>
> ACK
>
> Patch 6: sysdb: add sysdb_store_override
>
> ACK
>
> Patch 7: sysdb: sysdb_apply_default_override

Smal nitpick from our CI, after applying 7th path there is an error

src/db/sysdb_views.c: In function 'safe_original_attributes':
src/db/sysdb_views.c:539:13: error: implicit declaration of function 'sysdb_attrs_add_val_safe' [-Werror=implicit-function-declaration]
             ret = sysdb_attrs_add_val_safe(attrs, SYSDB_NAME_ALIAS,
             ^
cc1: all warnings being treated as errors

The solution is simple: just change order of patches.
0008-sysdb-add-sysdb_attrs_add_val_safe-and-sysdb_attrs_a.patch
0007-sysdb-sysdb_apply_default_override.patch

The function sysdb_attrs_add_val_safe was defined in 8th patch,
but was already used in 7th patch.

LS

[Sumit Bose]

On Wed, Oct 15, 2014 at 03:57:44PM +0200, Lukas Slebodnik wrote:
> On (15/10/14 15:37), Pavel Březina wrote:
> > On 10/14/2014 05:12 PM, Sumit Bose wrote:
> > > On Tue, Oct 14, 2014 at 11:24:04AM +0200, Sumit Bose wrote:
> > > > On Tue, Oct 14, 2014 at 11:16:28AM +0200, Sumit Bose wrote:
> > > > > On Wed, Oct 01, 2014 at 08:14:21PM +0200, Sumit Bose wrote:
> > > > > > On Mon, Sep 29, 2014 at 07:38:54PM +0200, Jakub Hrozek wrote:
> > > > > > >On Mon, Sep 29, 2014 at 03:00:44PM +0200, Sumit Bose wrote:
> > > > > > >>>Hi,
> > > > > > >>>I'm sending two more trivial debug patches to squash in. Otherwise its an
> > > > > > >>>code wise ack. I'm running some tests now.
> > > > > > >>>
> > > > > > >>
> > > > > > >>Thank you. Jakub, in case there are no other issues, can you add them
> > > > > > >>when committing the patches, or shall I send a new round?
> > > > > > >>
> > > > > > >>bye,
> > > > > > >>Sumit
> > > > > > >
> > > > > > >I will run the patches through the IPA CI engine with Tomas' help. If
> > > > > > >there are no regressions, I will squash in the patches and push, if
> > > > > > >there are, we would do another round.
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > this new series of patches contain fixes for various issues found by
> > > > > > Pavel, Jakub and Alexander. There is an additional 9th patch which
> > > > > > contains support for searching in the override values, e.g. if the user
> > > > > > name is overridden we have to first search in the override data and the
> > > > > > continue with the original object. See commit message for details.
> > > > >
> > > > > Please find attached the latest version of the patches with even more
> > > > > fixes for issue found by Alexander, Jakub and Pavel. I tired to keep the
> > > > > original structure of the patches but a new patch sneaked in which adds
> > > > > some new sysdb interface which allow to add attribute values with
> > > > > detection of duplicates.
> > > > >
> > > > > bye,
> > > > > Sumit
> > > >
> > > > I forgot the say that the patches now depend on the two patches I send
> > > > in the "[PATCHES] nss: add SSS_NSS_GETORIGBYNAME request" thread.
> > >
> > > This new version fixes some minor issues found by Coverity.
> >
> > Hi,

Pavel, Lukas, thank you for the review.
> > Patch 1: sysdb: add sysdb_update_view_name()
> > > static errno_t sysdb_get_view_name_ex(TALLOC_CTX *mem_ctx,
> > >                                      struct sysdb_ctx *sysdb,
> > >                                      char **_view_name,
> > >                                      bool *view_container_exists)
> > > {
> >
> > ...
> > > *_view_name = talloc_strdup(mem_ctx, tmp_str);
> >
> > I think it is possible to just steal the value since ldb uses talloc. But if
> > you want to strdup the string you should test for NULL.

good point, I steal it.
> > Do we really need view_container_exists? I think it is not possible to have a
> > view without a name and I would rather report an error in this case since it
> > basically means that db is corrupted.

I would prefer to keep it. You are right, currently the code always
creates the container with a name but I would like to be on the save
side and avoid issues when creating ing the container to a second time.
> > > ret = EOK;
> > >
> > > done:
> > >    talloc_free(tmp_ctx);
> > >    return ret;
> > > }
> > > errno_t sysdb_update_view_name(struct sysdb_ctx *sysdb,
> > >                               const char *view_name)
> > > {
> > >    errno_t ret;
> > >    TALLOC_CTX *tmp_ctx;
> > >    char *tmp_str;
> > >    bool view_container_exists = false;
> > >    bool add_view_name = false;
> > >    struct ldb_message *msg;
> >
> > I suppose this function is not yet completed? add_view_name is basically not
> > used since it is always true.

yes, it will be used when we allow to change the view.
> > > if (ret == EOK) {
> > >        if (strcmp(tmp_str, view_name) == 0) {
> > >            /* view name already known, mothing to do */
> >                                      ^ *typo*

fixed
> > >        DEBUG(SSSDBG_TRACE_ALL, "View name already in place.\n");
> > >        ret = EOK;
> > >        goto done;
> >
> > Patch 2: Add sdap_deref_search_with_filter_send()
> >
> > ACK
> >
> > Patch 3: IPA: make IPA ID context available to extdom client code
> >
> > ACK
> >
> > Patch 4: IPA: add view support and get view name
> >
> > ACK
> >
> > Do you think the list of options is now complete? The new options are still
> > missing in man page and config api.

added
> > Patch 5: views: add ipa_get_ad_override_send()
> >
> > ACK
> >
> > Patch 6: sysdb: add sysdb_store_override
> >
> > ACK
> >
> > Patch 7: sysdb: sysdb_apply_default_override
>
> Smal nitpick from our CI, after applying 7th path there is an error
>
> src/db/sysdb_views.c: In function 'safe_original_attributes':
> src/db/sysdb_views.c:539:13: error: implicit declaration of function 'sysdb_attrs_add_val_safe' [-Werror=implicit-function-declaration]
>              ret = sysdb_attrs_add_val_safe(attrs, SYSDB_NAME_ALIAS,
>              ^
> cc1: all warnings being treated as errors
>
> The solution is simple: just change order of patches.
> 0008-sysdb-add-sysdb_attrs_add_val_safe-and-sysdb_attrs_a.patch
> 0007-sysdb-sysdb_apply_default_override.patch
>
> The function sysdb_attrs_add_val_safe was defined in 8th patch,
> but was already used in 7th patch.

The order is switched now.

New version attached.

bye,
Sumit
> LS
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Březina]

On 10/16/2014 01:37 PM, Sumit Bose wrote:
> On Wed, Oct 15, 2014 at 03:57:44PM +0200, Lukas Slebodnik wrote:
> > On (15/10/14 15:37), Pavel Březina wrote:
> > > On 10/14/2014 05:12 PM, Sumit Bose wrote:
> > > > On Tue, Oct 14, 2014 at 11:24:04AM +0200, Sumit Bose wrote:
> > > > > On Tue, Oct 14, 2014 at 11:16:28AM +0200, Sumit Bose wrote:
> > > > > > On Wed, Oct 01, 2014 at 08:14:21PM +0200, Sumit Bose wrote:
> > > > > > > On Mon, Sep 29, 2014 at 07:38:54PM +0200, Jakub Hrozek wrote:
> > > > > > >> On Mon, Sep 29, 2014 at 03:00:44PM +0200, Sumit Bose wrote:
> > > > > > >>>> Hi,
> > > > > > >>>> I'm sending two more trivial debug patches to squash in. Otherwise its an
> > > > > > >>>> code wise ack. I'm running some tests now.
> > > > > > >>>>
> > > > > > >>>
> > > > > > >>> Thank you. Jakub, in case there are no other issues, can you add them
> > > > > > >>> when committing the patches, or shall I send a new round?
> > > > > > >>>
> > > > > > >>> bye,
> > > > > > >>> Sumit
> > > > > > >>
> > > > > > >> I will run the patches through the IPA CI engine with Tomas' help. If
> > > > > > >> there are no regressions, I will squash in the patches and push, if
> > > > > > >> there are, we would do another round.
> > > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > this new series of patches contain fixes for various issues found by
> > > > > > > Pavel, Jakub and Alexander. There is an additional 9th patch which
> > > > > > > contains support for searching in the override values, e.g. if the user
> > > > > > > name is overridden we have to first search in the override data and the
> > > > > > > continue with the original object. See commit message for details.
> > > > > >
> > > > > > Please find attached the latest version of the patches with even more
> > > > > > fixes for issue found by Alexander, Jakub and Pavel. I tired to keep the
> > > > > > original structure of the patches but a new patch sneaked in which adds
> > > > > > some new sysdb interface which allow to add attribute values with
> > > > > > detection of duplicates.
> > > > > >
> > > > > > bye,
> > > > > > Sumit
> > > > >
> > > > > I forgot the say that the patches now depend on the two patches I send
> > > > > in the "[PATCHES] nss: add SSS_NSS_GETORIGBYNAME request" thread.
> > > >
> > > > This new version fixes some minor issues found by Coverity.
> > >
> > > Hi,
>
> Pavel, Lukas, thank you for the review.
> > > Patch 1: sysdb: add sysdb_update_view_name()
> > > > static errno_t sysdb_get_view_name_ex(TALLOC_CTX *mem_ctx,
> > > >                                       struct sysdb_ctx *sysdb,
> > > >                                       char **_view_name,
> > > >                                       bool *view_container_exists)
> > > > {
> > >
> > > ...
> > > > *_view_name = talloc_strdup(mem_ctx, tmp_str);
> > >
> > > I think it is possible to just steal the value since ldb uses talloc. But if
> > > you want to strdup the string you should test for NULL.
>
> good point, I steal it.
> > > Do we really need view_container_exists? I think it is not possible to have a
> > > view without a name and I would rather report an error in this case since it
> > > basically means that db is corrupted.
>
> I would prefer to keep it. You are right, currently the code always
> creates the container with a name but I would like to be on the save
> side and avoid issues when creating ing the container to a second time.
> > > > ret = EOK;
> > > >
> > > > done:
> > > >     talloc_free(tmp_ctx);
> > > >     return ret;
> > > > }
> > > > errno_t sysdb_update_view_name(struct sysdb_ctx *sysdb,
> > > >                                const char *view_name)
> > > > {
> > > >     errno_t ret;
> > > >     TALLOC_CTX *tmp_ctx;
> > > >     char *tmp_str;
> > > >     bool view_container_exists = false;
> > > >     bool add_view_name = false;
> > > >     struct ldb_message *msg;
> > >
> > > I suppose this function is not yet completed? add_view_name is basically not
> > > used since it is always true.
>
> yes, it will be used when we allow to change the view.
> > > > if (ret == EOK) {
> > > >     if (strcmp(tmp_str, view_name) == 0) {
> > > >         /* view name already known, mothing to do */
> > >                                       ^ *typo*
>
> fixed
> > > >         DEBUG(SSSDBG_TRACE_ALL, "View name already in place.\n");
> > > >         ret = EOK;
> > > >         goto done;
> > >
> > > Patch 2: Add sdap_deref_search_with_filter_send()
> > >
> > > ACK
> > >
> > > Patch 3: IPA: make IPA ID context available to extdom client code
> > >
> > > ACK
> > >
> > > Patch 4: IPA: add view support and get view name
> > >
> > > ACK
> > >
> > > Do you think the list of options is now complete? The new options are still
> > > missing in man page and config api.
>
> added
> > > Patch 5: views: add ipa_get_ad_override_send()
> > >
> > > ACK
> > >
> > > Patch 6: sysdb: add sysdb_store_override
> > >
> > > ACK
> > >
> > > Patch 7: sysdb: sysdb_apply_default_override
> >
> > Smal nitpick from our CI, after applying 7th path there is an error
> >
> > src/db/sysdb_views.c: In function 'safe_original_attributes':
> > src/db/sysdb_views.c:539:13: error: implicit declaration of function 'sysdb_attrs_add_val_safe' [-Werror=implicit-function-declaration]
> >               ret = sysdb_attrs_add_val_safe(attrs, SYSDB_NAME_ALIAS,
> >               ^
> > cc1: all warnings being treated as errors
> >
> > The solution is simple: just change order of patches.
> > 0008-sysdb-add-sysdb_attrs_add_val_safe-and-sysdb_attrs_a.patch
> > 0007-sysdb-sysdb_apply_default_override.patch
> >
> > The function sysdb_attrs_add_val_safe was defined in 8th patch,
> > but was already used in 7th patch.
>
> The order is switched now.
>
> New version attached.
>
> bye,
> Sumit

Hi,
>                    <term>ipa_user_override_object_class (string)</term>
>                    <listitem>
>                        <para>
>                            Name if the objectclass for user overrides. It

^ *of*
>                            is use to determine if the found override object

^ *used*
>                            related to a user or a group.

^ *is related*
>                        </para>

The same typos are also in description of ipa_group_override_object_class.

Besides those typos it's a full ACK.

[Sumit Bose]

On Thu, Oct 16, 2014 at 04:39:12PM +0200, Pavel Březina wrote:
> On 10/16/2014 01:37 PM, Sumit Bose wrote:
> > On Wed, Oct 15, 2014 at 03:57:44PM +0200, Lukas Slebodnik wrote:
> > > On (15/10/14 15:37), Pavel Březina wrote:
> > > > On 10/14/2014 05:12 PM, Sumit Bose wrote:
> > > > > On Tue, Oct 14, 2014 at 11:24:04AM +0200, Sumit Bose wrote:
> > > > > > On Tue, Oct 14, 2014 at 11:16:28AM +0200, Sumit Bose wrote:
> > > > > > >On Wed, Oct 01, 2014 at 08:14:21PM +0200, Sumit Bose wrote:
> > > > > > >>On Mon, Sep 29, 2014 at 07:38:54PM +0200, Jakub Hrozek wrote:
> > > > > > >>>On Mon, Sep 29, 2014 at 03:00:44PM +0200, Sumit Bose wrote:
> > > > > > >>>>>Hi,
> > > > > > >>>>>I'm sending two more trivial debug patches to squash in. Otherwise its an
> > > > > > >>>>>code wise ack. I'm running some tests now.
> > > > > > >>>>>
> > > > > > >>>>
> > > > > > >>>>Thank you. Jakub, in case there are no other issues, can you add them
> > > > > > >>>>when committing the patches, or shall I send a new round?
> > > > > > >>>>
> > > > > > >>>>bye,
> > > > > > >>>>Sumit
> > > > > > >>>
> > > > > > >>>I will run the patches through the IPA CI engine with Tomas' help. If
> > > > > > >>>there are no regressions, I will squash in the patches and push, if
> > > > > > >>>there are, we would do another round.
> > > > > > >>
> > > > > > >>Hi,
> > > > > > >>
> > > > > > >>this new series of patches contain fixes for various issues found by
> > > > > > >>Pavel, Jakub and Alexander. There is an additional 9th patch which
> > > > > > >>contains support for searching in the override values, e.g. if the user
> > > > > > >>name is overridden we have to first search in the override data and the
> > > > > > >>continue with the original object. See commit message for details.
> > > > > > >
> > > > > > >Please find attached the latest version of the patches with even more
> > > > > > >fixes for issue found by Alexander, Jakub and Pavel. I tired to keep the
> > > > > > >original structure of the patches but a new patch sneaked in which adds
> > > > > > >some new sysdb interface which allow to add attribute values with
> > > > > > >detection of duplicates.
> > > > > > >
> > > > > > >bye,
> > > > > > >Sumit
> > > > > >
> > > > > > I forgot the say that the patches now depend on the two patches I send
> > > > > > in the "[PATCHES] nss: add SSS_NSS_GETORIGBYNAME request" thread.
> > > > >
> > > > > This new version fixes some minor issues found by Coverity.
> > > >
> > > > Hi,
> >
> > Pavel, Lukas, thank you for the review.
> > > > Patch 1: sysdb: add sysdb_update_view_name()
> > > > > static errno_t sysdb_get_view_name_ex(TALLOC_CTX *mem_ctx,
> > > > >                                      struct sysdb_ctx *sysdb,
> > > > >                                      char **_view_name,
> > > > >                                      bool *view_container_exists)
> > > > > {
> > > >
> > > > ...
> > > > > *_view_name = talloc_strdup(mem_ctx, tmp_str);
> > > >
> > > > I think it is possible to just steal the value since ldb uses talloc. But if
> > > > you want to strdup the string you should test for NULL.
> >
> > good point, I steal it.
> > > > Do we really need view_container_exists? I think it is not possible to have a
> > > > view without a name and I would rather report an error in this case since it
> > > > basically means that db is corrupted.
> >
> > I would prefer to keep it. You are right, currently the code always
> > creates the container with a name but I would like to be on the save
> > side and avoid issues when creating ing the container to a second time.
> > > > > ret = EOK;
> > > > >
> > > > > done:
> > > > >    talloc_free(tmp_ctx);
> > > > >    return ret;
> > > > > }
> > > > > errno_t sysdb_update_view_name(struct sysdb_ctx *sysdb,
> > > > >                               const char *view_name)
> > > > > {
> > > > >    errno_t ret;
> > > > >    TALLOC_CTX *tmp_ctx;
> > > > >    char *tmp_str;
> > > > >    bool view_container_exists = false;
> > > > >    bool add_view_name = false;
> > > > >    struct ldb_message *msg;
> > > >
> > > > I suppose this function is not yet completed? add_view_name is basically not
> > > > used since it is always true.
> >
> > yes, it will be used when we allow to change the view.
> > > > > if (ret == EOK) {
> > > > >        if (strcmp(tmp_str, view_name) == 0) {
> > > > >            /* view name already known, mothing to do */
> > > >                                      ^ *typo*
> >
> > fixed
> > > > >        DEBUG(SSSDBG_TRACE_ALL, "View name already in place.\n");
> > > > >        ret = EOK;
> > > > >        goto done;
> > > >
> > > > Patch 2: Add sdap_deref_search_with_filter_send()
> > > >
> > > > ACK
> > > >
> > > > Patch 3: IPA: make IPA ID context available to extdom client code
> > > >
> > > > ACK
> > > >
> > > > Patch 4: IPA: add view support and get view name
> > > >
> > > > ACK
> > > >
> > > > Do you think the list of options is now complete? The new options are still
> > > > missing in man page and config api.
> >
> > added
> > > > Patch 5: views: add ipa_get_ad_override_send()
> > > >
> > > > ACK
> > > >
> > > > Patch 6: sysdb: add sysdb_store_override
> > > >
> > > > ACK
> > > >
> > > > Patch 7: sysdb: sysdb_apply_default_override
> > >
> > > Smal nitpick from our CI, after applying 7th path there is an error
> > >
> > > src/db/sysdb_views.c: In function 'safe_original_attributes':
> > > src/db/sysdb_views.c:539:13: error: implicit declaration of function 'sysdb_attrs_add_val_safe' [-Werror=implicit-function-declaration]
> > >              ret = sysdb_attrs_add_val_safe(attrs, SYSDB_NAME_ALIAS,
> > >              ^
> > > cc1: all warnings being treated as errors
> > >
> > > The solution is simple: just change order of patches.
> > > 0008-sysdb-add-sysdb_attrs_add_val_safe-and-sysdb_attrs_a.patch
> > > 0007-sysdb-sysdb_apply_default_override.patch
> > >
> > > The function sysdb_attrs_add_val_safe was defined in 8th patch,
> > > but was already used in 7th patch.
> >
> > The order is switched now.
> >
> > New version attached.
> >
> > bye,
> > Sumit
>
> Hi,
> >                    <term>ipa_user_override_object_class (string)</term>
> >                    <listitem>
> >                        <para>
> >                            Name if the objectclass for user overrides. It
>                                     ^ *of*
> >                            is use to determine if the found override object
>                                   ^ *used*
> >                            related to a user or a group.
>                                ^ *is related*
> >                        </para>
>
> The same typos are also in description of ipa_group_override_object_class.
>
> Besides those typos it's a full ACK.

Thank you very much for the review. New version attached.

bye,
Sumit
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Březina]

On 10/16/2014 04:58 PM, Sumit Bose wrote:
> On Thu, Oct 16, 2014 at 04:39:12PM +0200, Pavel Březina wrote:
> > On 10/16/2014 01:37 PM, Sumit Bose wrote:
> > > On Wed, Oct 15, 2014 at 03:57:44PM +0200, Lukas Slebodnik wrote:
> > > > On (15/10/14 15:37), Pavel Březina wrote:
> > > > > On 10/14/2014 05:12 PM, Sumit Bose wrote:
> > > > > > On Tue, Oct 14, 2014 at 11:24:04AM +0200, Sumit Bose wrote:
> > > > > > > On Tue, Oct 14, 2014 at 11:16:28AM +0200, Sumit Bose wrote:
> > > > > > >> On Wed, Oct 01, 2014 at 08:14:21PM +0200, Sumit Bose wrote:
> > > > > > >>> On Mon, Sep 29, 2014 at 07:38:54PM +0200, Jakub Hrozek wrote:
> > > > > > >>>> On Mon, Sep 29, 2014 at 03:00:44PM +0200, Sumit Bose wrote:
> > > > > > >>>>>> Hi,
> > > > > > >>>>>> I'm sending two more trivial debug patches to squash in. Otherwise its an
> > > > > > >>>>>> code wise ack. I'm running some tests now.
> > > > > > >>>>>>
> > > > > > >>>>>
> > > > > > >>>>> Thank you. Jakub, in case there are no other issues, can you add them
> > > > > > >>>>> when committing the patches, or shall I send a new round?
> > > > > > >>>>>
> > > > > > >>>>> bye,
> > > > > > >>>>> Sumit
> > > > > > >>>>
> > > > > > >>>> I will run the patches through the IPA CI engine with Tomas' help. If
> > > > > > >>>> there are no regressions, I will squash in the patches and push, if
> > > > > > >>>> there are, we would do another round.
> > > > > > >>>
> > > > > > >>> Hi,
> > > > > > >>>
> > > > > > >>> this new series of patches contain fixes for various issues found by
> > > > > > >>> Pavel, Jakub and Alexander. There is an additional 9th patch which
> > > > > > >>> contains support for searching in the override values, e.g. if the user
> > > > > > >>> name is overridden we have to first search in the override data and the
> > > > > > >>> continue with the original object. See commit message for details.
> > > > > > >>
> > > > > > >> Please find attached the latest version of the patches with even more
> > > > > > >> fixes for issue found by Alexander, Jakub and Pavel. I tired to keep the
> > > > > > >> original structure of the patches but a new patch sneaked in which adds
> > > > > > >> some new sysdb interface which allow to add attribute values with
> > > > > > >> detection of duplicates.
> > > > > > >>
> > > > > > >> bye,
> > > > > > >> Sumit
> > > > > > >
> > > > > > > I forgot the say that the patches now depend on the two patches I send
> > > > > > > in the "[PATCHES] nss: add SSS_NSS_GETORIGBYNAME request" thread.
> > > > > >
> > > > > > This new version fixes some minor issues found by Coverity.
> > > > >
> > > > > Hi,
> > >
> > > Pavel, Lukas, thank you for the review.
> > > > > Patch 1: sysdb: add sysdb_update_view_name()
> > > > > > static errno_t sysdb_get_view_name_ex(TALLOC_CTX *mem_ctx,
> > > > > >                                       struct sysdb_ctx *sysdb,
> > > > > >                                       char **_view_name,
> > > > > >                                       bool *view_container_exists)
> > > > > > {
> > > > >
> > > > > ...
> > > > > > *_view_name = talloc_strdup(mem_ctx, tmp_str);
> > > > >
> > > > > I think it is possible to just steal the value since ldb uses talloc. But if
> > > > > you want to strdup the string you should test for NULL.
> > >
> > > good point, I steal it.
> > > > > Do we really need view_container_exists? I think it is not possible to have a
> > > > > view without a name and I would rather report an error in this case since it
> > > > > basically means that db is corrupted.
> > >
> > > I would prefer to keep it. You are right, currently the code always
> > > creates the container with a name but I would like to be on the save
> > > side and avoid issues when creating ing the container to a second time.
> > > > > > ret = EOK;
> > > > > >
> > > > > > done:
> > > > > >     talloc_free(tmp_ctx);
> > > > > >     return ret;
> > > > > > }
> > > > > > errno_t sysdb_update_view_name(struct sysdb_ctx *sysdb,
> > > > > >                                const char *view_name)
> > > > > > {
> > > > > >     errno_t ret;
> > > > > >     TALLOC_CTX *tmp_ctx;
> > > > > >     char *tmp_str;
> > > > > >     bool view_container_exists = false;
> > > > > >     bool add_view_name = false;
> > > > > >     struct ldb_message *msg;
> > > > >
> > > > > I suppose this function is not yet completed? add_view_name is basically not
> > > > > used since it is always true.
> > >
> > > yes, it will be used when we allow to change the view.
> > > > > > if (ret == EOK) {
> > > > > >     if (strcmp(tmp_str, view_name) == 0) {
> > > > > >         /* view name already known, mothing to do */
> > > > >                                       ^ *typo*
> > >
> > > fixed
> > > > > >         DEBUG(SSSDBG_TRACE_ALL, "View name already in place.\n");
> > > > > >         ret = EOK;
> > > > > >         goto done;
> > > > >
> > > > > Patch 2: Add sdap_deref_search_with_filter_send()
> > > > >
> > > > > ACK
> > > > >
> > > > > Patch 3: IPA: make IPA ID context available to extdom client code
> > > > >
> > > > > ACK
> > > > >
> > > > > Patch 4: IPA: add view support and get view name
> > > > >
> > > > > ACK
> > > > >
> > > > > Do you think the list of options is now complete? The new options are still
> > > > > missing in man page and config api.
> > >
> > > added
> > > > > Patch 5: views: add ipa_get_ad_override_send()
> > > > >
> > > > > ACK
> > > > >
> > > > > Patch 6: sysdb: add sysdb_store_override
> > > > >
> > > > > ACK
> > > > >
> > > > > Patch 7: sysdb: sysdb_apply_default_override
> > > >
> > > > Smal nitpick from our CI, after applying 7th path there is an error
> > > >
> > > > src/db/sysdb_views.c: In function 'safe_original_attributes':
> > > > src/db/sysdb_views.c:539:13: error: implicit declaration of function 'sysdb_attrs_add_val_safe' [-Werror=implicit-function-declaration]
> > > >               ret = sysdb_attrs_add_val_safe(attrs, SYSDB_NAME_ALIAS,
> > > >               ^
> > > > cc1: all warnings being treated as errors
> > > >
> > > > The solution is simple: just change order of patches.
> > > > 0008-sysdb-add-sysdb_attrs_add_val_safe-and-sysdb_attrs_a.patch
> > > > 0007-sysdb-sysdb_apply_default_override.patch
> > > >
> > > > The function sysdb_attrs_add_val_safe was defined in 8th patch,
> > > > but was already used in 7th patch.
> > >
> > > The order is switched now.
> > >
> > > New version attached.
> > >
> > > bye,
> > > Sumit
> >
> > Hi,
> > >                    <term>ipa_user_override_object_class (string)</term>
> > >                    <listitem>
> > >                        <para>
> > >                            Name if the objectclass for user overrides. It
> >                                      ^ *of*
> > >                            is use to determine if the found override object
> >                                    ^ *used*
> > >                            related to a user or a group.
> >                                 ^ *is related*
> > >                        </para>
> >
> > The same typos are also in description of ipa_group_override_object_class.
> >
> > Besides those typos it's a full ACK.
>
> Thank you very much for the review. New version attached.
>
> bye,
> Sumit

ACK to all.

[Jakub Hrozek]

On Thu, Oct 16, 2014 at 05:22:20PM +0200, Pavel Březina wrote:
> > Thank you very much for the review. New version attached.
> >
> > bye,
> > Sumit
>
> ACK to all.

Thank you both for the hard work on this feature. Pushed to master:

* ed4a9bd4d0f7fb359bed66a8d63a92e7be633aae
* 9c8db0a17a66c58c36966b17d004142a4aaace8d
* 9da27cbc7532f775afc411d809735760dd5294a7
* 8a2a503fa5c01ea037d28b7c902b8821a11084bd
* ca49ae1eee321751681e99f3ebe2547211db3bf6
* 0f3df54840ec9a050cc0b1b68269c3f28c859e64
* 08ab0d4ede41a1749e0bc26f78a37a4d10c20db8
* 00c283ca719717ed483958571982d0e9ff95c4b1
* 7d35c7e8c5d2684321be879f7ff67816d4b31f09
* 2ef62c64e7f07c8aced3f72850008ecb72860162

[Jakub Hrozek]

On Mon, Sep 29, 2014 at 01:01:32PM +0200, Sumit Bose wrote:
> From 76a1612288d2217157dd5ea7cb9745b0a0fa691a Mon Sep 17 00:00:00 2001
> From: Sumit Bose 
> Date: Tue, 16 Sep 2014 15:18:53 +0200
> Subject: [PATCH 1/8] sysdb: add sysdb_update_view_name()
>
> Makefile.am          |   1 +
>  src/db/sysdb.h       |  11 ++++
>  src/db/sysdb_views.c | 182 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 194 insertions(+)
>  create mode 100644 src/db/sysdb_views.c
>
> diff --git a/Makefile.am b/Makefile.am
> index 86a0572..bc93d34 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -664,6 +664,7 @@ libsss_util_la_SOURCES = \
>      src/db/sysdb_services.c \
>      src/db/sysdb_autofs.c \
>      src/db/sysdb_subdomains.c \
> src/db/sysdb_views.c \
> src/db/sysdb_ranges.c \
> src/db/sysdb_idmap.c \
> src/db/sysdb_gpo.c \
>
> diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> index 901b612..41fd3c5 100644
> --- a/src/db/sysdb.h
> +++ b/src/db/sysdb.h
> @@ -36,11 +36,13 @@
>  #define SYSDB_CUSTOM_CONTAINER "cn=custom"
>  #define SYSDB_NETGROUP_CONTAINER "cn=Netgroups"
>  #define SYSDB_RANGE_CONTAINER "cn=ranges"
> +#define SYSDB_VIEW_CONTAINER "cn=views"
>  #define SYSDB_TMPL_USER_BASE SYSDB_USERS_CONTAINER","SYSDB_DOM_BASE
>  #define SYSDB_TMPL_GROUP_BASE SYSDB_GROUPS_CONTAINER","SYSDB_DOM_BASE
>  #define SYSDB_TMPL_CUSTOM_BASE SYSDB_CUSTOM_CONTAINER","SYSDB_DOM_BASE
>  #define SYSDB_TMPL_NETGROUP_BASE SYSDB_NETGROUP_CONTAINER","SYSDB_DOM_BASE
>  #define SYSDB_TMPL_RANGE_BASE SYSDB_RANGE_CONTAINER","SYSDB_BASE
> +#define SYSDB_TMPL_VIEW_BASE SYSDB_VIEW_CONTAINER","SYSDB_BASE
>
> #define SYSDB_SUBDOMAIN_CLASS "subdomain"
>  #define SYSDB_USER_CLASS "user"
> @@ -138,6 +140,10 @@
>  #define SYSDB_DOMAIN_ID "domainID"
>  #define SYSDB_ID_RANGE_TYPE "idRangeType"
>
> +#define SYSDB_VIEW_CLASS "view"
> +#define SYSDB_VIEW_NAME "viewName"
> +#define SYSDB_DEFAULT_VIEW_NAME "default"

This should probably read "Default Trust View".