URL:
https://github.com/SSSD/sssd/pull/311
Title: #311: RESPONDER: Use fqnames as output when needed
jhrozek commented:
"""
On Tue, Jun 20, 2017 at 05:33:08AM -0700, fidencio wrote:
On Tue, Jun 20, 2017 at 11:33 AM, Jakub Hrozek
<notifications(a)github.com>
wrote:
> *@jhrozek* requested changes on this pull request.
>
> We should think about not modifying the sss_domain_info structure..
> ------------------------------
>
> In src/responder/common/cache_req/cache_req_domain.c
> <
https://github.com/SSSD/sssd/pull/311#discussion_r122925383>:
>
> > @@ -155,6 +161,14 @@ cache_req_domain_new_list_from_string_list(TALLOC_CTX
*mem_ctx,
> cr_domain->fqnames =
> cache_req_domain_use_fqnames(dom, enforce_non_fqnames);
>
> + /* when using the domain resolution order, using shortnames as input
> + * is allowed by default. However, we really want to use the fully
> + * qualified name as output in order to avoid conflicts whith users
> + * who have the very same name. */
> + if (resolution_order != NULL) {
> + cr_domain->domain->output_fqnames = true;
>
> I'm sorry I didn't notice this the first time I was reviewing the patch,
> but I don't like the cache_req request code modifying the domain property.
> Could we please do this instead at the time the resolution order is read?
>
Jakub, I'm not sure if I understand your suggestion here.
Although I really dislike playing with domain's property from the cache_req
code, I do believe a setter would be enough for making it cleaner.
Yeah, I don't like the side-effect of calling
`cache_req_domain_new_list_from_domain_resolution_order` and having the
domain property magically changed.
Could you please add a comment to
`cache_req_domain_new_list_from_domain_resolution_order` that this
happens? I also wonder if we should mark the domain parameter as
'output' with an underscore, but that's usually done for parameters that
the function constructs.
Setting those attributes in the place where we flatten the list (as it's
done with this patch) seems the saner approach to be taken (+ the setter
than has been introduced in the last pushed series).
Please, let me know what was your idea so I could, at least, try to follow
your suggestion in case you don't "buy" my approach.
> Additionally, I wonder if we could have a setter for the output_fqnames
> property. We've been wanting to make the sss_domain_info opaque precisely
> to control who can set what.
>
Done!
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <
https://github.com/SSSD/sssd/pull/311#pullrequestreview-45080418>, or mute
> the thread
>
<
https://github.com/notifications/unsubscribe-auth/AAG4euYkOQZoeo6dH4b2frs...
> .
>
--
You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub:
https://github.com/SSSD/sssd/pull/311#issuecomment-309738765
"""
See the full comment at
https://github.com/SSSD/sssd/pull/311#issuecomment-309991986