[PATCH] sss_cache: support for subdomains

[Michal Židek]

https://fedorahosted.org/sssd/ticket/1741

Patch is attached.

Thanks
Michal

[Jakub Hrozek]

On Mon, Apr 08, 2013 at 03:13:40PM +0200, Michal Židek wrote:
> https://fedorahosted.org/sssd/ticket/1741
>
> Patch is attached.
>
> Thanks
> Michal

Almost ack :)
> for (dinfo_list = tctx->domains; dinfo_list;
>     dinfo_list = get_next_domain(dinfo_list, false)) {
>    sysdb = dinfo_list->sysdb;
>    sysdb_update_subdomains(dinfo_list);

Please check the return value here, print a debug message on failure and
continue

Can you add a comment atop this block of code? It's not very easy to
understand:
>        if (main_domain) {
>            if (!dinfo->subdomains) {
>                /* This domain has no subdomains */
>                break;
>            }
>            main_domain = false;
>            dinfo = dinfo->subdomains;
>        } else {
>            dinfo = dinfo->next;
>        }
>    } while (dinfo);

Otherwise I'm fine with the code.

[Michal Židek]

On 04/10/2013 04:06 PM, Jakub Hrozek wrote:
> On Mon, Apr 08, 2013 at 03:13:40PM +0200, Michal Židek wrote:
> > https://fedorahosted.org/sssd/ticket/1741
> >
> > Patch is attached.
> >
> > Thanks
> > Michal
>
> Almost ack :)
> > for (dinfo_list = tctx->domains; dinfo_list;
> >     dinfo_list = get_next_domain(dinfo_list, false)) {
> >    sysdb = dinfo_list->sysdb;
> >    sysdb_update_subdomains(dinfo_list);
>
> Please check the return value here, print a debug message on failure and
> continue
>
> Can you add a comment atop this block of code? It's not very easy to
> understand:
> >        if (main_domain) {
> >            if (!dinfo->subdomains) {
> >                /* This domain has no subdomains */
> >                break;
> >            }
> >            main_domain = false;
> >            dinfo = dinfo->subdomains;
> >        } else {
> >            dinfo = dinfo->next;
> >        }
> >    } while (dinfo);
>
> Otherwise I'm fine with the code.

New patch attached.

Thanks
Michal

[Jakub Hrozek]

On Wed, Apr 10, 2013 at 05:55:45PM +0200, Michal Židek wrote:
> New patch attached.
>
> Thanks
> Michal
>        use_name = talloc_asprintf(tmp_ctx, "%s@%s", name_lower,
>                                   domain_lower);
>        if (use_name == NULL) {
>            DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory\n"));
>            ret = ENOMEM;
>            goto done;
>        }
>    } else {
>        use_name = parsed_name;
>    }

Sorry, one more thing I missed earlier..you can't really hardcode the
format like this, you need to use the fq_fmt from sss_names_ctx.

[Michal Židek]

On 04/11/2013 08:45 AM, Jakub Hrozek wrote:
> On Wed, Apr 10, 2013 at 05:55:45PM +0200, Michal Židek wrote:
> > New patch attached.
> >
> > Thanks
> > Michal
> >        use_name = talloc_asprintf(tmp_ctx, "%s@%s", name_lower,
> >                                   domain_lower);
> >        if (use_name == NULL) {
> >            DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory\n"));
> >            ret = ENOMEM;
> >            goto done;
> >         }
> >     } else {
> >        use_name = parsed_name;
> >    }
>
> Sorry, one more thing I missed earlier..you can't really hardcode the
> format like this, you need to use the fq_fmt from sss_names_ctx.

New patch attached.

Thanks
Michal

[Jakub Hrozek]

On Thu, Apr 11, 2013 at 11:35:52AM +0200, Michal Židek wrote:
> New patch attached.
>
> Thanks
> Michal
>    if (IS_SUBDOMAIN(dinfo)) {
>        name_lower = sss_tc_utf8_str_tolower(tmp_ctx, parsed_name);
>        domain_lower = sss_tc_utf8_str_tolower(tmp_ctx, parsed_domain);
>        if (!name_lower || !domain_lower) {
>            DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory\n"));
>            ret = ENOMEM;
>            goto done;
>        }

One last nitpick -- while this code is technically correct now, it might not
be in future, we should also check the case_sensitive flag in dinfo and
only lowercase based on the value of the flag.

[Michal Židek]

On 04/12/2013 04:57 PM, Jakub Hrozek wrote:
> On Thu, Apr 11, 2013 at 11:35:52AM +0200, Michal Židek wrote:
> > New patch attached.
> >
> > Thanks
> > Michal
> >    if (IS_SUBDOMAIN(dinfo)) {
> >        name_lower = sss_tc_utf8_str_tolower(tmp_ctx, parsed_name);
> >        domain_lower = sss_tc_utf8_str_tolower(tmp_ctx, parsed_domain);
> >        if (!name_lower || !domain_lower) {
> >            DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory\n"));
> >            ret = ENOMEM;
> >            goto done;
> >        }
>
> One last nitpick -- while this code is technically correct now, it might not
> be in future, we should also check the case_sensitive flag in dinfo and
> only lowercase based on the value of the flag.

Ok. I have also slightly changed the code flow in update_filter() 
function to make it more readable and added new parameter to force case 
sensibility (for autofs maps). New patch attached.

Thanks
Michal

[Jakub Hrozek]

On Fri, Apr 12, 2013 at 08:35:23PM +0200, Michal Židek wrote:
> On 04/12/2013 04:57 PM, Jakub Hrozek wrote:
> > On Thu, Apr 11, 2013 at 11:35:52AM +0200, Michal Židek wrote:
> > > New patch attached.
> > >
> > > Thanks
> > > Michal
> > >    if (IS_SUBDOMAIN(dinfo)) {
> > >        name_lower = sss_tc_utf8_str_tolower(tmp_ctx, parsed_name);
> > >        domain_lower = sss_tc_utf8_str_tolower(tmp_ctx, parsed_domain);
> > >        if (!name_lower || !domain_lower) {
> > >            DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory\n"));
> > >            ret = ENOMEM;
> > >            goto done;
> > >        }
> >
> > One last nitpick -- while this code is technically correct now, it might not
> > be in future, we should also check the case_sensitive flag in dinfo and
> > only lowercase based on the value of the flag.
>
> Ok. I have also slightly changed the code flow in update_filter()
> function to make it more readable and added new parameter to force
> case sensibility (for autofs maps). New patch attached.
>
> Thanks
> Michal

It seems that fqdns do not work with this patch. Also
get_next_domain(dom, true) can maybe be used in the domain loop.

[Michal Židek]

On 04/15/2013 12:44 PM, Jakub Hrozek wrote:
> On Fri, Apr 12, 2013 at 08:35:23PM +0200, Michal Židek wrote:
> > On 04/12/2013 04:57 PM, Jakub Hrozek wrote:
> > > On Thu, Apr 11, 2013 at 11:35:52AM +0200, Michal Židek wrote:
> > > > New patch attached.
> > > >
> > > > Thanks
> > > > Michal
> > > >    if (IS_SUBDOMAIN(dinfo)) {
> > > >        name_lower = sss_tc_utf8_str_tolower(tmp_ctx, parsed_name);
> > > >        domain_lower = sss_tc_utf8_str_tolower(tmp_ctx, parsed_domain);
> > > >        if (!name_lower || !domain_lower) {
> > > >            DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory\n"));
> > > >            ret = ENOMEM;
> > > >            goto done;
> > > >        }
> > >
> > > One last nitpick -- while this code is technically correct now, it might not
> > > be in future, we should also check the case_sensitive flag in dinfo and
> > > only lowercase based on the value of the flag.
> >
> > Ok. I have also slightly changed the code flow in update_filter()
> > function to make it more readable and added new parameter to force
> > case sensibility (for autofs maps). New patch attached.
> >
> > Thanks
> > Michal
>
> It seems that fqdns do not work with this patch. Also

Yes. Sorry, the last change I did broke fqdns for users from native domains.
> get_next_domain(dom, true) can maybe be used in the domain loop.

Great. I could remove the ugly part with "main_domain". Thanks for 
catching this.

I am also sending the patch with the annoying message to this thread (it 
was already acked in another thread, but depends on this patch with 
subdomains).

Thanks
Michal

[Jakub Hrozek]

On Mon, Apr 15, 2013 at 01:01:21PM +0200, Michal Židek wrote:
> On 04/15/2013 12:44 PM, Jakub Hrozek wrote:
> > On Fri, Apr 12, 2013 at 08:35:23PM +0200, Michal Židek wrote:
> > > On 04/12/2013 04:57 PM, Jakub Hrozek wrote:
> > > > On Thu, Apr 11, 2013 at 11:35:52AM +0200, Michal Židek wrote:
> > > > > New patch attached.
> > > > >
> > > > > Thanks
> > > > > Michal
> > > > >    if (IS_SUBDOMAIN(dinfo)) {
> > > > >        name_lower = sss_tc_utf8_str_tolower(tmp_ctx, parsed_name);
> > > > >        domain_lower = sss_tc_utf8_str_tolower(tmp_ctx, parsed_domain);
> > > > >        if (!name_lower || !domain_lower) {
> > > > >            DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory\n"));
> > > > >            ret = ENOMEM;
> > > > >            goto done;
> > > > >        }
> > > >
> > > > One last nitpick -- while this code is technically correct now, it might not
> > > > be in future, we should also check the case_sensitive flag in dinfo and
> > > > only lowercase based on the value of the flag.
> > >
> > > Ok. I have also slightly changed the code flow in update_filter()
> > > function to make it more readable and added new parameter to force
> > > case sensibility (for autofs maps). New patch attached.
> > >
> > > Thanks
> > > Michal
> >
> > It seems that fqdns do not work with this patch. Also
>
> Yes. Sorry, the last change I did broke fqdns for users from native domains.
> > get_next_domain(dom, true) can maybe be used in the domain loop.
>
> Great. I could remove the ugly part with "main_domain". Thanks for
> catching this.
>
> I am also sending the patch with the annoying message to this thread
> (it was already acked in another thread, but depends on this patch
> with subdomains).
>
> Thanks
> Michal

Ack

[Jakub Hrozek]

On Mon, Apr 15, 2013 at 02:50:20PM +0200, Jakub Hrozek wrote:
> Ack

Pushed to master.