[PATCH] DNS site support 1st wave - generic SRV lookup plugin

[Pavel Březina]

Hi,
this patchset takes the current SRV lookup code and converts it into a
plugin. Next waves will add custom plugin for IPA and AD.

Patch 1:
Introduce new plugin interface.

Patch 2:
First SRV lookup plugin.

Patch 3:
Replaces current code with a plugin code. I originally wanted to remove
resolve_srv_* functions completely and call the plugin in
fo_resolve_service_send() directly but that proved to be harder and
more error sensitive than I expected.

This got me thinking, we should create a refactoring ticket for the
failover code. It has become quite complex and hard to read and
understand because it lacks a lot of comments. Also many things there
seems very hackish to me.

Patch 4:
Set SRV lookup plugin for all providers. This is done as a separate
patch because it will be reverted once each provider has its own plugin.

I also would like to discuss where we can set the plugin in the future.
At the moment we have one failover context for the whole backend. But
the backend can be configured to run several different providers at the
same time. The plugin itself is made per provider - you have different
plugin for IPA and different for LDAP.

I think we should have failover context per provider. My idea is to
create a new module constructor e.g. sssm_ipa_init(). This will be
called only once before all other sssm_ipa_*_init() functions and it
will initialize IPA-wise failover context.

We can also use it to initialize sdap_id_ctx instead of calling
sssm_ipa_id_init from other places.

[Jakub Hrozek]

On Fri, Mar 22, 2013 at 02:47:31PM +0100, Pavel Březina wrote:
> Hi,
> this patchset takes the current SRV lookup code and converts it into a
> plugin. Next waves will add custom plugin for IPA and AD.
>
> Patch 1:
> Introduce new plugin interface.
>
> Patch 2:
> First SRV lookup plugin.
>
> Patch 3:
> Replaces current code with a plugin code. I originally wanted to remove
> resolve_srv_* functions completely and call the plugin in
> fo_resolve_service_send() directly but that proved to be harder and
> more error sensitive than I expected.
>
> This got me thinking, we should create a refactoring ticket for the
> failover code. It has become quite complex and hard to read and
> understand because it lacks a lot of comments. Also many things there
> seems very hackish to me.

Feel free to create that ticket, I agree that the current failover code
has grown too much..but keep in mind that many of the "hacks" are in fact
special cases for all kinds of weird errors.

My idea was to decouple the caching logic in the failover to a separate
layer. So resolver would always talk to DNS or whatever back end there
was, then there would be a resolver cache and finally fail over that
would only manage states of servers and ports.

Is there anything else that bothers you with the current failover
architecture?
> Patch 4:
> Set SRV lookup plugin for all providers. This is done as a separate
> patch because it will be reverted once each provider has its own plugin.
>
> I also would like to discuss where we can set the plugin in the future.
> At the moment we have one failover context for the whole backend. But
> the backend can be configured to run several different providers at the
> same time. The plugin itself is made per provider - you have different
> plugin for IPA and different for LDAP.
>
> I think we should have failover context per provider. My idea is to
> create a new module constructor e.g. sssm_ipa_init(). This will be
> called only once before all other sssm_ipa_*_init() functions and it
> will initialize IPA-wise failover context.

In theory there may be a case where we need this functionality, yes. I
can't think of any use-case right now, but this may be a better
architecture going forward, especially as we are adding more providers
like sudo or autofs that are only implemented with some back ends.
> We can also use it to initialize sdap_id_ctx instead of calling
> sssm_ipa_id_init from other places.

Please see comments inline.
> From 4391f7f47317f3c5664f577b833ef91d3f8e15e2 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Tue, 19 Mar 2013 15:53:44 +0100
> Subject: [PATCH 1/4] DNS sites support - SRV lookup plugin interface
>
> https://fedorahosted.org/sssd/ticket/1032
>
> Introduces two new error codes:
> ERR_DNS_SRV_NOT_FOUND
> ERR_DNS_SRV_LOOKUP_ERROR
>
> src/providers/fail_over.c | 18 +++++++++++++++++
>  src/providers/fail_over.h | 51 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/util_errors.c    |  2 ++
>  src/util/util_errors.h    |  2 ++
>  4 files changed, 73 insertions(+)
>
> diff --git a/src/providers/fail_over.c b/src/providers/fail_over.c
> index e7c44174ded773a8e3bb99dc436c45d4e8ca277d..660fe8261aa279735af2b8d9d31596acbad19810 100644
> --- a/src/providers/fail_over.c
> +++ b/src/providers/fail_over.c
> @@ -55,6 +55,10 @@ struct fo_ctx {
>      struct server_common *server_common_list;
>  struct fo_options *opts;
> srv_lookup_plugin_send_t srv_send_fn;
> srv_lookup_plugin_recv_t srv_recv_fn;
> void *srv_pvt;

The design page we agreed on describes that these callbacks should have
been in the resolv context. Any reason to move them to fo_ctx ?

The failover should be providing logic for moving between servers and
ports and just call "resolve a service". The logic on how to resolve a
service should be in the resolver I think. Also see the fo_server_info
structure -- this seems exactly like code that belongs to resolver.
> };
>
> struct fo_service {
> @@ -1591,3 +1595,17 @@ bool fo_svc_has_server(struct fo_service *service, struct fo_server *server)
>  return false;
>
> }
>
> +void fo_set_srv_lookup_plugin(struct fo_ctx *ctx,
>                          srv_lookup_plugin_send_t send_fn,
>                          srv_lookup_plugin_recv_t recv_fn,
>                          void *pvt)
>
> +{
> if (ctx == NULL) {
>    return;
> }
> ctx->srv_send_fn = send_fn;
> ctx->srv_recv_fn = recv_fn;
> ctx->srv_pvt = talloc_steal(ctx, pvt);
>
> +}
> diff --git a/src/providers/fail_over.h b/src/providers/fail_over.h
> index 1ad081e78c866390a1a345feacc5c0899adf91a4..1ff06688237b5fde1d78878f45bd87f7ca1df8cf 100644
> --- a/src/providers/fail_over.h
> +++ b/src/providers/fail_over.h
> @@ -198,4 +198,55 @@ void fo_reset_services(struct fo_ctx *fo_ctx);
>
> bool fo_svc_has_server(struct fo_service *service, struct fo_server *server);
>
> +/* SRV lookup plugin interface */
>
> +struct fo_server_info {
> char *host;
> int port;
>
> +};

OK, in the design we talked about reusing ares_srv structure..you are
right we might as well create a wrapper, but it should be in resolver,
not failover. A failover server (a bunch of services with status) is a
completely different concept than resolver server (host name an port).
> @@ -63,6 +63,8 @@ enum sssd_errors {
>      ERR_ACCOUNT_EXPIRED,
>      ERR_PASSWORD_EXPIRED,
>      ERR_ACCESS_DENIED,
> ERR_DNS_SRV_NOT_FOUND,
> ERR_DNS_SRV_LOOKUP_ERROR,
> ERR_LAST            /* ALWAYS LAST */
>
> };

Wouldn't it be better to create generic SRV lookup error codes that
could be reused also for AD site discovery (which is not DNS).
> diff --git a/src/providers/fail_over.h b/src/providers/fail_over.h
> index 1ff06688237b5fde1d78878f45bd87f7ca1df8cf..0ab87d3230408b420a3d78e30b335ab9902c7407 100644
> --- a/src/providers/fail_over.h
> +++ b/src/providers/fail_over.h
> @@ -249,4 +249,30 @@ void fo_set_srv_lookup_plugin(struct fo_ctx *ctx,
>                                srv_lookup_plugin_recv_t recv_fn,
>                                void *pvt);
>
> +/* Simple SRV lookup plugin */
>
> +struct resolve_srv_simple_ctx;

The namespacing seems wrong to me. There is a resolv_ namespace in the
resolver code and fo_ namespace in failover code, this is third one and
similar to resolv_ ... very confusing.
> +struct resolve_srv_simple_ctx *
> +resolve_srv_simple_context_init(TALLOC_CTX *mem_ctx,
>                            struct resolv_ctx *resolv_ctx,
>                            enum host_database *host_dbs,
>                            enum restrict_family family_order,
>                            char *sssd_domain);

I think this is a good idea to couple together the databases, resolv_ctx
and the family order because in most cases, they are simply used together,
while allowing the low level resolver calls to specify all these parameters.

But this new structure should be owned by the user of the resolver, which
is currently back end and might be provider in the future.
> +struct tevent_req *resolve_srv_simple_send(TALLOC_CTX *mem_ctx,
>                                       struct tevent_context *ev,
>                                       const char *service,
>                                       const char *protocol,
>                                       const char *discovery_domain,
>                                       void *pvt);

I don't like the _simple name either, sorry. If there is a more advanced
static interface, then the simple should be called just _srv_send/recv
and the advanced _ext, I think. The reason is to have nice and readable
names for the public interface.
> +errno_t resolve_srv_simple_recv(TALLOC_CTX *mem_ctx,
>                            struct tevent_req *req,
>                            char **_dns_domain,
>                            struct fo_server_info **_primary_servers,
>                            size_t *_num_primary_servers,
>                            struct fo_server_info **_backup_servers,
>                            size_t *_num_backup_servers);
> +struct resolve_srv_domain_state {
> char *fqdn;
> char hostname[HOST_NAME_MAX];
>
> +};
>
> +static void resolve_srv_domain_done(struct tevent_req *subreq);
>
> +static struct tevent_req *
> +resolve_srv_domain_send(TALLOC_CTX *mem_ctx,
>                    struct tevent_context *ev,
>                    struct resolv_ctx *resolv_ctx,
>                    enum host_database *host_dbs,
>                    enum restrict_family family_order)
>
> +{

I'm not opposed to splitting this code to a separate request and move
away from failover, but because it only calls resolv_ functions directly
it should be placed in the resolver code.
> struct resolve_srv_domain_state *state = NULL;
> struct tevent_req *req = NULL;
> struct tevent_req *subreq = NULL;
> errno_t ret;
> req = tevent_req_create(mem_ctx, &state,
>                        struct resolve_srv_domain_state);
> if (req == NULL) {
>    DEBUG(SSSDBG_CRIT_FAILURE, ("tevent_req_create() failed\n"));
>    return NULL;
> }
> state->fqdn = NULL;
> ret = gethostname(state->hostname, HOST_NAME_MAX);
> if (ret) {
>    ret = errno;
>    DEBUG(SSSDBG_CRIT_FAILURE, ("gethostname() failed: [%d]: %s\n",
>                                ret, strerror(ret)));
>    goto immediately;
> }

I know that we have the same situation in the current code, but the host
name should be an input parameter. Some back ends allow you to override the
client host name (ipa_hostname parameter comes to mind).

<snip>
> +struct resolve_srv_state {
> struct tevent_context *ev;
> struct resolv_ctx *resolv_ctx;
> char *service;
> char *protocol;
> char *detected_domain;
> const char **discovery_domains;
> int domain_index;
> struct fo_server_info *servers;
> size_t num_servers;
>
> +};

Please move this whole request to resolver. Feel free to split the
resolver code into more modules and introduce a structure analogous to
the fo_server_info one.
> +static void resolve_srv_step1_done(struct tevent_req *subreq);

Ugh, step1 is not the greatest name :)

I think we should resume review of the other patches when we settle
which is the best place to implement the plugin architecture.

[Jakub Hrozek]

On Mon, Mar 25, 2013 at 05:18:27PM +0100, Jakub Hrozek wrote:
> On Fri, Mar 22, 2013 at 02:47:31PM +0100, Pavel Březina wrote:
> > Hi,
> > this patchset takes the current SRV lookup code and converts it into a
> > plugin. Next waves will add custom plugin for IPA and AD.
> >
> > Patch 1:
> > Introduce new plugin interface.
> >
> > Patch 2:
> > First SRV lookup plugin.
> >
> > Patch 3:
> > Replaces current code with a plugin code. I originally wanted to remove
> > resolve_srv_* functions completely and call the plugin in
> > fo_resolve_service_send() directly but that proved to be harder and
> > more error sensitive than I expected.
> >
> > This got me thinking, we should create a refactoring ticket for the
> > failover code. It has become quite complex and hard to read and
> > understand because it lacks a lot of comments. Also many things there
> > seems very hackish to me.
>
> Feel free to create that ticket, I agree that the current failover code
> has grown too much..but keep in mind that many of the "hacks" are in fact
> special cases for all kinds of weird errors.
>
> My idea was to decouple the caching logic in the failover to a separate
> layer. So resolver would always talk to DNS or whatever back end there
> was, then there would be a resolver cache and finally fail over that
> would only manage states of servers and ports.
>
> Is there anything else that bothers you with the current failover
> architecture?
> > Patch 4:
> > Set SRV lookup plugin for all providers. This is done as a separate
> > patch because it will be reverted once each provider has its own plugin.
> >
> > I also would like to discuss where we can set the plugin in the future.
> > At the moment we have one failover context for the whole backend. But
> > the backend can be configured to run several different providers at the
> > same time. The plugin itself is made per provider - you have different
> > plugin for IPA and different for LDAP.
> >
> > I think we should have failover context per provider. My idea is to
> > create a new module constructor e.g. sssm_ipa_init(). This will be
> > called only once before all other sssm_ipa_*_init() functions and it
> > will initialize IPA-wise failover context.
>
> In theory there may be a case where we need this functionality, yes. I
> can't think of any use-case right now, but this may be a better
> architecture going forward, especially as we are adding more providers
> like sudo or autofs that are only implemented with some back ends.
> > We can also use it to initialize sdap_id_ctx instead of calling
> > sssm_ipa_id_init from other places.
>
> Please see comments inline.
> > From 4391f7f47317f3c5664f577b833ef91d3f8e15e2 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Tue, 19 Mar 2013 15:53:44 +0100
> > Subject: [PATCH 1/4] DNS sites support - SRV lookup plugin interface
> >
> > https://fedorahosted.org/sssd/ticket/1032
> >
> > Introduces two new error codes:
> > ERR_DNS_SRV_NOT_FOUND
> > ERR_DNS_SRV_LOOKUP_ERROR
> >
> > src/providers/fail_over.c | 18 +++++++++++++++++
> >  src/providers/fail_over.h | 51 +++++++++++++++++++++++++++++++++++++++++++++++
> >  src/util/util_errors.c    |  2 ++
> >  src/util/util_errors.h    |  2 ++
> >  4 files changed, 73 insertions(+)
> >
> > diff --git a/src/providers/fail_over.c b/src/providers/fail_over.c
> > index e7c44174ded773a8e3bb99dc436c45d4e8ca277d..660fe8261aa279735af2b8d9d31596acbad19810 100644
> > --- a/src/providers/fail_over.c
> > +++ b/src/providers/fail_over.c
> > @@ -55,6 +55,10 @@ struct fo_ctx {
> >      struct server_common *server_common_list;
> >  struct fo_options *opts;
> > srv_lookup_plugin_send_t srv_send_fn;
> > srv_lookup_plugin_recv_t srv_recv_fn;
> > void *srv_pvt;
>
> The design page we agreed on describes that these callbacks should have
> been in the resolv context. Any reason to move them to fo_ctx ?
>
> The failover should be providing logic for moving between servers and
> ports and just call "resolve a service". The logic on how to resolve a
> service should be in the resolver I think. Also see the fo_server_info
> structure -- this seems exactly like code that belongs to resolver.

We discussed the approach with Pavel a bit and I'd like to gather more
opinions. I still prefer the basic SRV plugin to be placed in resolver
directly to keep the current layered architecture:

be_fo -> fo -> resolv

I think it still makes sense because the failover (and the backend
specific be_fo) not only always resolve, but also act as kind of a cache
and in many cases would simply return the current server because it's
still valid.

The plugins, on the other hand, should have no knowledge of "servers",
"ports" and other failover concepts and go straight to the DNS or AD
server.

The only thing with the current design[1] that introduced new concept to
the resolver is that the plugin would return "primary" and "secondary"
results, while previously the resolver just returned results and let the
failover sort them out. I think we can either just accept this, or make
the results returned from the resolver a bit more generic to allow
storing private data (such as primary/backup) in the results.

But in general I still think the most generic SRV plugin belongs to the
resolver and the back ends would be allowed to override it with some
specific plugins of theirs.

Are there any thoughts on placing the plugins to resolver versus
failover?

[1]
https://fedorahosted.org/sssd/wiki/DesignDocs/ActiveDirectoryDNSSites

[Pavel Březina]

On 03/27/2013 09:57 AM, Jakub Hrozek wrote:
> On Mon, Mar 25, 2013 at 05:18:27PM +0100, Jakub Hrozek wrote:
> > On Fri, Mar 22, 2013 at 02:47:31PM +0100, Pavel Březina wrote:
> > > Hi, this patchset takes the current SRV lookup code and converts
> > > it into a plugin. Next waves will add custom plugin for IPA and
> > > AD.
> > >
> > > Patch 1: Introduce new plugin interface.
> > >
> > > Patch 2: First SRV lookup plugin.
> > >
> > > Patch 3: Replaces current code with a plugin code. I originally
> > > wanted to remove resolve_srv_* functions completely and call the
> > > plugin in fo_resolve_service_send() directly but that proved to
> > > be harder and more error sensitive than I expected.
> > >
> > > This got me thinking, we should create a refactoring ticket for
> > > the failover code. It has become quite complex and hard to read
> > > and understand because it lacks a lot of comments. Also many
> > > things there seems very hackish to me.
> >
> > Feel free to create that ticket, I agree that the current failover
> > code has grown too much..but keep in mind that many of the "hacks"
> > are in fact special cases for all kinds of weird errors.
> >
> > My idea was to decouple the caching logic in the failover to a
> > separate layer. So resolver would always talk to DNS or whatever
> > back end there was, then there would be a resolver cache and
> > finally fail over that would only manage states of servers and
> > ports.
> >
> > Is there anything else that bothers you with the current failover
> > architecture?
> > > Patch 4: Set SRV lookup plugin for all providers. This is done as
> > > a separate patch because it will be reverted once each provider
> > > has its own plugin.
> > >
> > > I also would like to discuss where we can set the plugin in the
> > > future. At the moment we have one failover context for the whole
> > > backend. But the backend can be configured to run several
> > > different providers at the same time. The plugin itself is made
> > > per provider - you have different plugin for IPA and different
> > > for LDAP.
> > >
> > > I think we should have failover context per provider. My idea is
> > > to create a new module constructor e.g. sssm_ipa_init(). This
> > > will be called only once before all other sssm_ipa_*_init()
> > > functions and it will initialize IPA-wise failover context.
> >
> > In theory there may be a case where we need this functionality,
> > yes. I can't think of any use-case right now, but this may be a
> > better architecture going forward, especially as we are adding more
> > providers like sudo or autofs that are only implemented with some
> > back ends.
> > > We can also use it to initialize sdap_id_ctx instead of calling
> > > sssm_ipa_id_init from other places.
> >
> > Please see comments inline.
> > > From 4391f7f47317f3c5664f577b833ef91d3f8e15e2 Mon Sep 17 00:00:00
> > > 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
> > >  Date: Tue, 19 Mar 2013 15:53:44 +0100
> > > Subject: [PATCH 1/4] DNS sites support - SRV lookup plugin
> > > interface
> > >
> > > https://fedorahosted.org/sssd/ticket/1032
> > >
> > > Introduces two new error codes: - ERR_DNS_SRV_NOT_FOUND -
> > > ERR_DNS_SRV_LOOKUP_ERROR --- src/providers/fail_over.c | 18
> > > +++++++++++++++++ src/providers/fail_over.h | 51
> > > +++++++++++++++++++++++++++++++++++++++++++++++
> > > src/util/util_errors.c    |  2 ++ src/util/util_errors.h    |  2
> > > ++ 4 files changed, 73 insertions(+)
> > >
> > > diff --git a/src/providers/fail_over.c
> > > b/src/providers/fail_over.c index
> > > e7c44174ded773a8e3bb99dc436c45d4e8ca277d..660fe8261aa279735af2b8d9d31596acbad19810
> > > 100644 --- a/src/providers/fail_over.c +++
> > > b/src/providers/fail_over.c @@ -55,6 +55,10 @@ struct fo_ctx {
> > > struct server_common *server_common_list;
> > >
> > > struct fo_options *opts; + +    srv_lookup_plugin_send_t
> > > srv_send_fn; +    srv_lookup_plugin_recv_t srv_recv_fn; +    void
> > > *srv_pvt;
> >
> > The design page we agreed on describes that these callbacks should
> > have been in the resolv context. Any reason to move them to fo_ctx
> > ?
> >
> > The failover should be providing logic for moving between servers
> > and ports and just call "resolve a service". The logic on how to
> > resolve a service should be in the resolver I think. Also see the
> > fo_server_info structure -- this seems exactly like code that
> > belongs to resolver.
>
> We discussed the approach with Pavel a bit and I'd like to gather
> more opinions. I still prefer the basic SRV plugin to be placed in
> resolver directly to keep the current layered architecture:
>
> be_fo -> fo -> resolv
>
> I think it still makes sense because the failover (and the backend
> specific be_fo) not only always resolve, but also act as kind of a
> cache and in many cases would simply return the current server
> because it's still valid.
>
> The plugins, on the other hand, should have no knowledge of
> "servers", "ports" and other failover concepts and go straight to the
> DNS or AD server.
>
> The only thing with the current design[1] that introduced new concept
> to the resolver is that the plugin would return "primary" and
> "secondary" results, while previously the resolver just returned
> results and let the failover sort them out. I think we can either
> just accept this, or make the results returned from the resolver a
> bit more generic to allow storing private data (such as
> primary/backup) in the results.

Storing private data as part of a result - wag.
> But in general I still think the most generic SRV plugin belongs to
> the resolver and the back ends would be allowed to override it with
> some specific plugins of theirs.
>
> Are there any thoughts on placing the plugins to resolver versus
> failover?
>
> [1]
> https://fedorahosted.org/sssd/wiki/DesignDocs/ActiveDirectoryDNSSites

I semi-agree that it doesn't belong to fail over, but I don't want to
put it inside resolver either.

Resolver is something that I see as a separate library inside SSSD tree.
It should not have any knowledge of SSSD what so ever. It is a tevent
wrapper around c-ares and should contain only low level functions for
resolving names and DNS queries.

Plugin
1) returns list of primary and backup server
- it has to, the plugin is the only thing that can categorized servers
2) may or may not communicate with DNS (e.g. AD plugin will send CLDAP
ping)

The resolver part of the plugin does not belong to the fail over. Yes.
But in addition, concept of primary and backup server is something that
should be only known to the fail over. The way how is fail over done
now, I'm inclined to put it in fail over.

Later, we can place the plugin architecture inside a module that knows
about SSSD and is placed above resolver. You said you want to decouple
fail over to fail over -> cache -> resolver. This is nice idea.

The plugin architecture can be put inside the cache, because cache is
responsible for resolving expired uris and _srv_ and it does not really
say that is has to use resolver directly.

[Jakub Hrozek]

On Wed, Mar 27, 2013 at 11:22:49AM +0100, Pavel Březina wrote:
> Later, we can place the plugin architecture inside a module that knows
> about SSSD and is placed above resolver. You said you want to decouple
> fail over to fail over -> cache -> resolver. This is nice idea.
>
> The plugin architecture can be put inside the cache, because cache is
> responsible for resolving expired uris and _srv_ and it does not really
> say that is has to use resolver directly.

OK, this would be the best approach. After more discussion on #sssd with
Pavel and Simo we agreed that Pavel will carry on with this approach for
the time being.

The only two changes in these patches would be better namespacing of the
functions used and unifying the resolver initialization with what I'm
working on wrt dynamic DNS updates.

In the next release, we will create the additional layer and move the
plugins there. I moved https://fedorahosted.org/sssd/ticket/1083 which is
tracking that effort to NEEDS_TRIAGE so we can put it into an appropriate
milestone tomorrow.

[Pavel Březina]

On 03/27/2013 03:00 PM, Jakub Hrozek wrote:
> On Wed, Mar 27, 2013 at 11:22:49AM +0100, Pavel Březina wrote:
> > Later, we can place the plugin architecture inside a module that
> > knows about SSSD and is placed above resolver. You said you want to
> > decouple fail over to fail over -> cache -> resolver. This is nice
> > idea.
> >
> > The plugin architecture can be put inside the cache, because cache
> > is responsible for resolving expired uris and _srv_ and it does not
> > really say that is has to use resolver directly.
>
> OK, this would be the best approach. After more discussion on #sssd
> with Pavel and Simo we agreed that Pavel will carry on with this
> approach for the time being.
>
> The only two changes in these patches would be better namespacing of
> the functions used and unifying the resolver initialization with what
> I'm working on wrt dynamic DNS updates.
>
> In the next release, we will create the additional layer and move
> the plugins there. I moved https://fedorahosted.org/sssd/ticket/1083
> which is tracking that effort to NEEDS_TRIAGE so we can put it into
> an appropriate milestone tomorrow.

Hi,
I'm sending new patch set.

I created async_resolv_utils.c and put there functions for domain
detection and for resolving SRV with fallback discovery domains. This is
similar to resolve_srv_send and resolve_get_domain_send from the first
patchset.

resolve_srv_send() was renamed to fo_discover_srv_send() and it wraps
resolv_discover_srv_send() so that it returns fo_server_info instead of
ares.

The plugin was renamed to fo_resolve_srv_dns_send(). It is a little bit
more complicated since it performs domain detection (originally in
resolve_srv_send()) if the domain is not provided from fail over.

Hostname is passed via plugin context. It is currently retrieved via
gethostname() so the current behaviour does not change. Later, when the
plugin initialization will be moved to provider code, the provider will
decide what hostname will be used (ipa_hostname for example).

[Pavel Březina]

On 04/02/2013 03:55 PM, Pavel Březina wrote:
> On 03/27/2013 03:00 PM, Jakub Hrozek wrote:
> > On Wed, Mar 27, 2013 at 11:22:49AM +0100, Pavel Březina wrote:
> > > Later, we can place the plugin architecture inside a module that
> > > knows about SSSD and is placed above resolver. You said you want to
> > > decouple fail over to fail over -> cache -> resolver. This is nice
> > > idea.
> > >
> > > The plugin architecture can be put inside the cache, because cache
> > > is responsible for resolving expired uris and _srv_ and it does not
> > > really say that is has to use resolver directly.
> >
> > OK, this would be the best approach. After more discussion on #sssd
> > with Pavel and Simo we agreed that Pavel will carry on with this
> > approach for the time being.
> >
> > The only two changes in these patches would be better namespacing of
> > the functions used and unifying the resolver initialization with what
> > I'm working on wrt dynamic DNS updates.
> >
> > In the next release, we will create the additional layer and move
> > the plugins there. I moved https://fedorahosted.org/sssd/ticket/1083
> > which is tracking that effort to NEEDS_TRIAGE so we can put it into
> > an appropriate milestone tomorrow.
>
> Hi,
> I'm sending new patch set.
>
> I created async_resolv_utils.c and put there functions for domain
> detection and for resolving SRV with fallback discovery domains. This is
> similar to resolve_srv_send and resolve_get_domain_send from the first
> patchset.
>
> resolve_srv_send() was renamed to fo_discover_srv_send() and it wraps
> resolv_discover_srv_send() so that it returns fo_server_info instead of
> ares.
>
> The plugin was renamed to fo_resolve_srv_dns_send(). It is a little bit
> more complicated since it performs domain detection (originally in
> resolve_srv_send()) if the domain is not provided from fail over.
>
> Hostname is passed via plugin context. It is currently retrieved via
> gethostname() so the current behaviour does not change. Later, when the
> plugin initialization will be moved to provider code, the provider will
> decide what hostname will be used (ipa_hostname for example).

Rebased on top of latest be_res patches.

[Pavel Březina]

On 04/03/2013 11:48 AM, Pavel Březina wrote:
> On 04/02/2013 03:55 PM, Pavel Březina wrote:
> > On 03/27/2013 03:00 PM, Jakub Hrozek wrote:
> > > On Wed, Mar 27, 2013 at 11:22:49AM +0100, Pavel Březina wrote:
> > > > Later, we can place the plugin architecture inside a module that
> > > > knows about SSSD and is placed above resolver. You said you want to
> > > > decouple fail over to fail over -> cache -> resolver. This is nice
> > > > idea.
> > > >
> > > > The plugin architecture can be put inside the cache, because cache
> > > > is responsible for resolving expired uris and _srv_ and it does not
> > > > really say that is has to use resolver directly.
> > >
> > > OK, this would be the best approach. After more discussion on #sssd
> > > with Pavel and Simo we agreed that Pavel will carry on with this
> > > approach for the time being.
> > >
> > > The only two changes in these patches would be better namespacing of
> > > the functions used and unifying the resolver initialization with what
> > > I'm working on wrt dynamic DNS updates.
> > >
> > > In the next release, we will create the additional layer and move
> > > the plugins there. I moved https://fedorahosted.org/sssd/ticket/1083
> > > which is tracking that effort to NEEDS_TRIAGE so we can put it into
> > > an appropriate milestone tomorrow.
> >
> > Hi,
> > I'm sending new patch set.
> >
> > I created async_resolv_utils.c and put there functions for domain
> > detection and for resolving SRV with fallback discovery domains. This is
> > similar to resolve_srv_send and resolve_get_domain_send from the first
> > patchset.
> >
> > resolve_srv_send() was renamed to fo_discover_srv_send() and it wraps
> > resolv_discover_srv_send() so that it returns fo_server_info instead of
> > ares.
> >
> > The plugin was renamed to fo_resolve_srv_dns_send(). It is a little bit
> > more complicated since it performs domain detection (originally in
> > resolve_srv_send()) if the domain is not provided from fail over.
> >
> > Hostname is passed via plugin context. It is currently retrieved via
> > gethostname() so the current behaviour does not change. Later, when the
> > plugin initialization will be moved to provider code, the provider will
> > decide what hostname will be used (ipa_hostname for example).
>
> Rebased on top of latest be_res patches.

When working on IPA plugin I've done some changes to this code. Since it 
is still review pending, I have squashed the changes to this patch set.

1. all SRV plugin related things are in a separate header file named 
fail_over_srv.h. This way we don't have to #include the whole fail over 
when developing new plugin. The only thing which remained in fail_over.h 
is fo_set_srv_lookup_plugin(), because it should remain hidden for 
providers, see #2

2. be_fo_set_srv_lookup_plugin() was added, to keep the fail over opaque 
for providers.

3. the original sixth patch (set plugin for all providers) was meant to 
be reverted, once we agreed on further steps. Now it is completely 
rewritten to final solution on which we agreed with Jakub.

I have the IPA plugin already written (not tested yet), so I believe 
this is the final version if there won't be any review comments.

[Pavel Březina]

On 04/05/2013 12:41 PM, Pavel Březina wrote:
> On 04/03/2013 11:48 AM, Pavel Březina wrote:
> > On 04/02/2013 03:55 PM, Pavel Březina wrote:
> > > On 03/27/2013 03:00 PM, Jakub Hrozek wrote:
> > > > On Wed, Mar 27, 2013 at 11:22:49AM +0100, Pavel Březina wrote:
> > > > > Later, we can place the plugin architecture inside a module that
> > > > > knows about SSSD and is placed above resolver. You said you want to
> > > > > decouple fail over to fail over -> cache -> resolver. This is nice
> > > > > idea.
> > > > >
> > > > > The plugin architecture can be put inside the cache, because cache
> > > > > is responsible for resolving expired uris and _srv_ and it does not
> > > > > really say that is has to use resolver directly.
> > > >
> > > > OK, this would be the best approach. After more discussion on #sssd
> > > > with Pavel and Simo we agreed that Pavel will carry on with this
> > > > approach for the time being.
> > > >
> > > > The only two changes in these patches would be better namespacing of
> > > > the functions used and unifying the resolver initialization with what
> > > > I'm working on wrt dynamic DNS updates.
> > > >
> > > > In the next release, we will create the additional layer and move
> > > > the plugins there. I moved https://fedorahosted.org/sssd/ticket/1083
> > > > which is tracking that effort to NEEDS_TRIAGE so we can put it into
> > > > an appropriate milestone tomorrow.
> > >
> > > Hi,
> > > I'm sending new patch set.
> > >
> > > I created async_resolv_utils.c and put there functions for domain
> > > detection and for resolving SRV with fallback discovery domains. This is
> > > similar to resolve_srv_send and resolve_get_domain_send from the first
> > > patchset.
> > >
> > > resolve_srv_send() was renamed to fo_discover_srv_send() and it wraps
> > > resolv_discover_srv_send() so that it returns fo_server_info instead of
> > > ares.
> > >
> > > The plugin was renamed to fo_resolve_srv_dns_send(). It is a little bit
> > > more complicated since it performs domain detection (originally in
> > > resolve_srv_send()) if the domain is not provided from fail over.
> > >
> > > Hostname is passed via plugin context. It is currently retrieved via
> > > gethostname() so the current behaviour does not change. Later, when the
> > > plugin initialization will be moved to provider code, the provider will
> > > decide what hostname will be used (ipa_hostname for example).
> >
> > Rebased on top of latest be_res patches.
>
> When working on IPA plugin I've done some changes to this code. Since it
> is still review pending, I have squashed the changes to this patch set.
> all SRV plugin related things are in a separate header file named
>
> fail_over_srv.h. This way we don't have to #include the whole fail over
> when developing new plugin. The only thing which remained in fail_over.h
> is fo_set_srv_lookup_plugin(), because it should remain hidden for
> providers, see #2
> be_fo_set_srv_lookup_plugin() was added, to keep the fail over opaque
>
> for providers.
> the original sixth patch (set plugin for all providers) was meant to
>
> be reverted, once we agreed on further steps. Now it is completely
> rewritten to final solution on which we agreed with Jakub.
>
> I have the IPA plugin already written (not tested yet), so I believe
> this is the final version if there won't be any review comments.

//lists.fedorahosted.org/mailman/listinfo/sssd-devel

OK. One more time :-)
I have slightly enhanced debug messages.

[Pavel Březina]

On 04/05/2013 01:43 PM, Pavel Březina wrote:
> On 04/05/2013 12:41 PM, Pavel Březina wrote:
> > On 04/03/2013 11:48 AM, Pavel Březina wrote:
> > > On 04/02/2013 03:55 PM, Pavel Březina wrote:
> > > > On 03/27/2013 03:00 PM, Jakub Hrozek wrote:
> > > > > On Wed, Mar 27, 2013 at 11:22:49AM +0100, Pavel Březina wrote:
> > > > > > Later, we can place the plugin architecture inside a module that
> > > > > > knows about SSSD and is placed above resolver. You said you want to
> > > > > > decouple fail over to fail over -> cache -> resolver. This is nice
> > > > > > idea.
> > > > > >
> > > > > > The plugin architecture can be put inside the cache, because cache
> > > > > > is responsible for resolving expired uris and _srv_ and it does not
> > > > > > really say that is has to use resolver directly.
> > > > >
> > > > > OK, this would be the best approach. After more discussion on #sssd
> > > > > with Pavel and Simo we agreed that Pavel will carry on with this
> > > > > approach for the time being.
> > > > >
> > > > > The only two changes in these patches would be better namespacing of
> > > > > the functions used and unifying the resolver initialization with what
> > > > > I'm working on wrt dynamic DNS updates.
> > > > >
> > > > > In the next release, we will create the additional layer and move
> > > > > the plugins there. I moved https://fedorahosted.org/sssd/ticket/1083
> > > > > which is tracking that effort to NEEDS_TRIAGE so we can put it into
> > > > > an appropriate milestone tomorrow.
> > > >
> > > > Hi,
> > > > I'm sending new patch set.
> > > >
> > > > I created async_resolv_utils.c and put there functions for domain
> > > > detection and for resolving SRV with fallback discovery domains.
> > > > This is
> > > > similar to resolve_srv_send and resolve_get_domain_send from the first
> > > > patchset.
> > > >
> > > > resolve_srv_send() was renamed to fo_discover_srv_send() and it wraps
> > > > resolv_discover_srv_send() so that it returns fo_server_info instead of
> > > > ares.
> > > >
> > > > The plugin was renamed to fo_resolve_srv_dns_send(). It is a little bit
> > > > more complicated since it performs domain detection (originally in
> > > > resolve_srv_send()) if the domain is not provided from fail over.
> > > >
> > > > Hostname is passed via plugin context. It is currently retrieved via
> > > > gethostname() so the current behaviour does not change. Later, when the
> > > > plugin initialization will be moved to provider code, the provider will
> > > > decide what hostname will be used (ipa_hostname for example).
> > >
> > > Rebased on top of latest be_res patches.
> >
> > When working on IPA plugin I've done some changes to this code. Since it
> > is still review pending, I have squashed the changes to this patch set.
> > all SRV plugin related things are in a separate header file named
> >
> > fail_over_srv.h. This way we don't have to #include the whole fail over
> > when developing new plugin. The only thing which remained in fail_over.h
> > is fo_set_srv_lookup_plugin(), because it should remain hidden for
> > providers, see #2
> > be_fo_set_srv_lookup_plugin() was added, to keep the fail over opaque
> >
> > for providers.
> > the original sixth patch (set plugin for all providers) was meant to
> >
> > be reverted, once we agreed on further steps. Now it is completely
> > rewritten to final solution on which we agreed with Jakub.
> >
> > I have the IPA plugin already written (not tested yet), so I believe
> > this is the final version if there won't be any review comments.
>
> //lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> OK. One more time :-)
> I have slightly enhanced debug messages.

I forgot to check if a server is already present in the list. The fifth
patch was split in two.

[Jakub Hrozek]

On Mon, Apr 08, 2013 at 11:55:15AM +0200, Pavel Březina wrote:
> On 04/05/2013 01:43 PM, Pavel Březina wrote:
> > On 04/05/2013 12:41 PM, Pavel Březina wrote:
> > > On 04/03/2013 11:48 AM, Pavel Březina wrote:
> > > > On 04/02/2013 03:55 PM, Pavel Březina wrote:
> > > > > On 03/27/2013 03:00 PM, Jakub Hrozek wrote:
> > > > > > On Wed, Mar 27, 2013 at 11:22:49AM +0100, Pavel Březina wrote:
> > > > > > >Later, we can place the plugin architecture inside a module that
> > > > > > >knows about SSSD and is placed above resolver. You said you want to
> > > > > > >decouple fail over to fail over -> cache -> resolver. This is nice
> > > > > > >idea.
> > > > > > >
> > > > > > >The plugin architecture can be put inside the cache, because cache
> > > > > > >is responsible for resolving expired uris and _srv_ and it does not
> > > > > > >really say that is has to use resolver directly.
> > > > > >
> > > > > > OK, this would be the best approach. After more discussion on #sssd
> > > > > > with Pavel and Simo we agreed that Pavel will carry on with this
> > > > > > approach for the time being.
> > > > > >
> > > > > > The only two changes in these patches would be better namespacing of
> > > > > > the functions used and unifying the resolver initialization with what
> > > > > > I'm working on wrt dynamic DNS updates.
> > > > > >
> > > > > > In the next release, we will create the additional layer and move
> > > > > > the plugins there. I moved https://fedorahosted.org/sssd/ticket/1083
> > > > > > which is tracking that effort to NEEDS_TRIAGE so we can put it into
> > > > > > an appropriate milestone tomorrow.
> > > > >
> > > > > Hi,
> > > > > I'm sending new patch set.
> > > > >
> > > > > I created async_resolv_utils.c and put there functions for domain
> > > > > detection and for resolving SRV with fallback discovery domains.
> > > > > This is
> > > > > similar to resolve_srv_send and resolve_get_domain_send from the first
> > > > > patchset.
> > > > >
> > > > > resolve_srv_send() was renamed to fo_discover_srv_send() and it wraps
> > > > > resolv_discover_srv_send() so that it returns fo_server_info instead of
> > > > > ares.
> > > > >
> > > > > The plugin was renamed to fo_resolve_srv_dns_send(). It is a little bit
> > > > > more complicated since it performs domain detection (originally in
> > > > > resolve_srv_send()) if the domain is not provided from fail over.
> > > > >
> > > > > Hostname is passed via plugin context. It is currently retrieved via
> > > > > gethostname() so the current behaviour does not change. Later, when the
> > > > > plugin initialization will be moved to provider code, the provider will
> > > > > decide what hostname will be used (ipa_hostname for example).
> > > >
> > > > Rebased on top of latest be_res patches.
> > >
> > > When working on IPA plugin I've done some changes to this code. Since it
> > > is still review pending, I have squashed the changes to this patch set.
> > > all SRV plugin related things are in a separate header file named
> > >
> > > fail_over_srv.h. This way we don't have to #include the whole fail over
> > > when developing new plugin. The only thing which remained in fail_over.h
> > > is fo_set_srv_lookup_plugin(), because it should remain hidden for
> > > providers, see #2
> > > be_fo_set_srv_lookup_plugin() was added, to keep the fail over opaque
> > >
> > > for providers.
> > > the original sixth patch (set plugin for all providers) was meant to
> > >
> > > be reverted, once we agreed on further steps. Now it is completely
> > > rewritten to final solution on which we agreed with Jakub.
> > >
> > > I have the IPA plugin already written (not tested yet), so I believe
> > > this is the final version if there won't be any review comments.
> >
> > //lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > OK. One more time :-)
> > I have slightly enhanced debug messages.
>
> I forgot to check if a server is already present in the list. The fifth
> patch was split in two.

I will review the code but for starters the failover suite is failing:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000406879 in fo_add_server_to_list (to_list=0x632790, check_list=0x0, server=0x633000, 
    service_name=0x632810 "ntp") at /home/remote/jhrozek/devel/sssd/src/providers/fail_over.c:682
682         const char *debug_name = server->common->name;
Missing separate debuginfos, use: debuginfo-install libattr-2.4.46-7.fc18.x86_64 libbasicobjects-0.1.0-0.fc18.x86_64 libcollection-0.6.2-0.fc18.x86_64 libini_config-1.0.0.1-0.fc18.x86_64 libref_array-0.1.3-0.fc18.x86_64
(gdb) bt
#0  0x0000000000406879 in fo_add_server_to_list (to_list=0x632790, check_list=0x0, server=0x633000, 
    service_name=0x632810 "ntp") at /home/remote/jhrozek/devel/sssd/src/providers/fail_over.c:682
#1  0x0000000000406ee8 in fo_add_server (service=0x632760, name=0x0, port=123, user_data=0x0, primary=
    true) at /home/remote/jhrozek/devel/sssd/src/providers/fail_over.c:762
#2  0x0000000000403765 in test_fo_resolve_service (_i=0)
    at /home/remote/jhrozek/devel/sssd/src/tests/fail_over-tests.c:241
#3  0x00007ffff7bd7f82 in tcase_run_tfun_nofork (sr=sr@entry=0x61f220, tc=tc@entry=0x61f030, i=i@entry=
    0, tfun=0x61f1f0, tfun=0x61f1f0) at check_run.c:332
#4  0x00007ffff7bd8396 in srunner_iterate_tcase_tfuns (tc=0x61f030, sr=0x61f220) at check_run.c:192
#5  srunner_run_tcase (tc=0x61f030, sr=0x61f220) at check_run.c:318
#6  srunner_iterate_suites (tcname=0x0, sname=0x0, sr=0x61f220, print_mode=<optimized out>)
    at check_run.c:161
#7  srunner_run (sr=0x61f220, sname=0x0, tcname=0x0, print_mode=<optimized out>) at check_run.c:596
#8  0x0000000000403b8f in main (argc=1, argv=0x7fffffffdd28)
    at /home/remote/jhrozek/devel/sssd/src/tests/fail_over-tests.c:321

[Jakub Hrozek]

On Mon, Apr 08, 2013 at 11:55:15AM +0200, Pavel Březina wrote:
> On 04/05/2013 01:43 PM, Pavel Březina wrote:
> > On 04/05/2013 12:41 PM, Pavel Březina wrote:
> > > On 04/03/2013 11:48 AM, Pavel Březina wrote:
> > > > On 04/02/2013 03:55 PM, Pavel Březina wrote:
> > > > > On 03/27/2013 03:00 PM, Jakub Hrozek wrote:
> > > > > > On Wed, Mar 27, 2013 at 11:22:49AM +0100, Pavel Březina wrote:
> > > > > > >Later, we can place the plugin architecture inside a module that
> > > > > > >knows about SSSD and is placed above resolver. You said you want to
> > > > > > >decouple fail over to fail over -> cache -> resolver. This is nice
> > > > > > >idea.
> > > > > > >
> > > > > > >The plugin architecture can be put inside the cache, because cache
> > > > > > >is responsible for resolving expired uris and _srv_ and it does not
> > > > > > >really say that is has to use resolver directly.
> > > > > >
> > > > > > OK, this would be the best approach. After more discussion on #sssd
> > > > > > with Pavel and Simo we agreed that Pavel will carry on with this
> > > > > > approach for the time being.
> > > > > >
> > > > > > The only two changes in these patches would be better namespacing of
> > > > > > the functions used and unifying the resolver initialization with what
> > > > > > I'm working on wrt dynamic DNS updates.
> > > > > >
> > > > > > In the next release, we will create the additional layer and move
> > > > > > the plugins there. I moved https://fedorahosted.org/sssd/ticket/1083
> > > > > > which is tracking that effort to NEEDS_TRIAGE so we can put it into
> > > > > > an appropriate milestone tomorrow.
> > > > >
> > > > > Hi,
> > > > > I'm sending new patch set.
> > > > >
> > > > > I created async_resolv_utils.c and put there functions for domain
> > > > > detection and for resolving SRV with fallback discovery domains.
> > > > > This is
> > > > > similar to resolve_srv_send and resolve_get_domain_send from the first
> > > > > patchset.
> > > > >
> > > > > resolve_srv_send() was renamed to fo_discover_srv_send() and it wraps
> > > > > resolv_discover_srv_send() so that it returns fo_server_info instead of
> > > > > ares.
> > > > >
> > > > > The plugin was renamed to fo_resolve_srv_dns_send(). It is a little bit
> > > > > more complicated since it performs domain detection (originally in
> > > > > resolve_srv_send()) if the domain is not provided from fail over.
> > > > >
> > > > > Hostname is passed via plugin context. It is currently retrieved via
> > > > > gethostname() so the current behaviour does not change. Later, when the
> > > > > plugin initialization will be moved to provider code, the provider will
> > > > > decide what hostname will be used (ipa_hostname for example).
> > > >
> > > > Rebased on top of latest be_res patches.
> > >
> > > When working on IPA plugin I've done some changes to this code. Since it
> > > is still review pending, I have squashed the changes to this patch set.
> > > all SRV plugin related things are in a separate header file named
> > >
> > > fail_over_srv.h. This way we don't have to #include the whole fail over
> > > when developing new plugin. The only thing which remained in fail_over.h
> > > is fo_set_srv_lookup_plugin(), because it should remain hidden for
> > > providers, see #2
> > > be_fo_set_srv_lookup_plugin() was added, to keep the fail over opaque
> > >
> > > for providers.
> > > the original sixth patch (set plugin for all providers) was meant to
> > >
> > > be reverted, once we agreed on further steps. Now it is completely
> > > rewritten to final solution on which we agreed with Jakub.
> > >
> > > I have the IPA plugin already written (not tested yet), so I believe
> > > this is the final version if there won't be any review comments.
> >
> > //lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > OK. One more time :-)
> > I have slightly enhanced debug messages.
>
> I forgot to check if a server is already present in the list. The fifth
> patch was split in two.

Patch 0001: resolv: add resolv_get_domain request to resolv utils
Mostly OK, can you just explain this part of the code?

152     dns_domain = talloc_strdup(state, dns_domain);
153     if (dns_domain == NULL) {
154         return ENOMEM;
155     }
156 
157     *_dns_domain = talloc_steal(mem_ctx, dns_domain);
158 
159     return EOK;

Isn't it better to just:
    *_dns_domain = talloc_strdup(mem_ctx, dns_domain);
?

Patch 0002: resolv: add resolv_discover_srv request to resolv utils
Is it necessary to strdup the input variables? It might be safer in
case this request outlives the request that created it but in most cases
it's not really needed. The discovery_domain, on the other hand is
neither checked not duped.

Patch 0003: DNS sites support - SRV lookup plugin interface
Code-wise looks good, but I have two questions related to the behaviour.

Why can't the setter overwrite the plugin interface when it's already
set? Isn't is nice for cases like LDAP where LDAP might set its generic
plugin first and then IPA which uses LDAP underneath would reset them?

Patch 0004: DNS sites support - SRV DNS lookup plugin
Please do not use constructs like this unless EOK is expected:

66 immediately:
67     if (ret == EOK) {
68         tevent_req_done(req);
69     } else {

At the very least it's confusing Coverity and showing up as "dead code".

Same question about strdup-ing input parameters as for patch #2.

I'm not really fond of the name "be_fo_set_standard_srv_lookup_plugin"
-- standard by what means? What about using the same naming standard as
in fail_over_srv.c and using be_fo_set_dns_srv_lookup_plugin ?

Also since be_fo_set_standard_srv_lookup_plugin can fail and then the
failover would not really be usable, it should return errno and users
should check it.

Patch 0005: fail over - add function to insert multiple servers to the
list
Ack

Patch 0006:  DNS sites support - replace SRV lookup code with a plugin
call
Did you check if the code can be compiled with this patch? If it can,
then it's fine that the lookups don't work but I would prefer the
patches to compile on their own so that git-bisect is usable.

Otherwise ack.

Patch 0007: DNS sites support - use SRV DNS lookup plugin in all providers
Mostly ack, but when I looked at how the host name is selected in LDAP
initialization, it seemed to be that this could be moved down the stack
either to be_fo_set_standard_srv_lookup_plugin or even lower to
resolv_utils. Anyway, looks ugly in the generic LDAP code :)

Patch 0008: DNS sites support - make fo_discover_srv request visible
I don't get the point of this patch, why not make the functions pubilc
from the start?

So far I haven't tested the patches, just read the code to give some
feedback. I'll reply again when I've tested it.

There is quite some new functionality in this patchset that would be
great to have covered by unit-tests. I realize we need to finish the
code first and foremost, so maybe we could file tickets (trivial or
minor) and fix them in 1.10 post-beta? But I would like to start
building the coverage, if we don't even file the ticket, we'll forget
completely.

[Jakub Hrozek]

On Mon, Apr 08, 2013 at 04:51:47PM +0200, Jakub Hrozek wrote:
> On Mon, Apr 08, 2013 at 11:55:15AM +0200, Pavel Březina wrote:
> > On 04/05/2013 01:43 PM, Pavel Březina wrote:
> > > On 04/05/2013 12:41 PM, Pavel Březina wrote:
> > > > On 04/03/2013 11:48 AM, Pavel Březina wrote:
> > > > > On 04/02/2013 03:55 PM, Pavel Březina wrote:
> > > > > > On 03/27/2013 03:00 PM, Jakub Hrozek wrote:
> > > > > > >On Wed, Mar 27, 2013 at 11:22:49AM +0100, Pavel Březina wrote:
> > > > > > >>Later, we can place the plugin architecture inside a module that
> > > > > > >>knows about SSSD and is placed above resolver. You said you want to
> > > > > > >>decouple fail over to fail over -> cache -> resolver. This is nice
> > > > > > >>idea.
> > > > > > >>
> > > > > > >>The plugin architecture can be put inside the cache, because cache
> > > > > > >>is responsible for resolving expired uris and _srv_ and it does not
> > > > > > >>really say that is has to use resolver directly.
> > > > > > >
> > > > > > >OK, this would be the best approach. After more discussion on #sssd
> > > > > > >with Pavel and Simo we agreed that Pavel will carry on with this
> > > > > > >approach for the time being.
> > > > > > >
> > > > > > >The only two changes in these patches would be better namespacing of
> > > > > > >the functions used and unifying the resolver initialization with what
> > > > > > >I'm working on wrt dynamic DNS updates.
> > > > > > >
> > > > > > >In the next release, we will create the additional layer and move
> > > > > > >the plugins there. I moved https://fedorahosted.org/sssd/ticket/1083
> > > > > > >which is tracking that effort to NEEDS_TRIAGE so we can put it into
> > > > > > >an appropriate milestone tomorrow.
> > > > > >
> > > > > > Hi,
> > > > > > I'm sending new patch set.
> > > > > >
> > > > > > I created async_resolv_utils.c and put there functions for domain
> > > > > > detection and for resolving SRV with fallback discovery domains.
> > > > > > This is
> > > > > > similar to resolve_srv_send and resolve_get_domain_send from the first
> > > > > > patchset.
> > > > > >
> > > > > > resolve_srv_send() was renamed to fo_discover_srv_send() and it wraps
> > > > > > resolv_discover_srv_send() so that it returns fo_server_info instead of
> > > > > > ares.
> > > > > >
> > > > > > The plugin was renamed to fo_resolve_srv_dns_send(). It is a little bit
> > > > > > more complicated since it performs domain detection (originally in
> > > > > > resolve_srv_send()) if the domain is not provided from fail over.
> > > > > >
> > > > > > Hostname is passed via plugin context. It is currently retrieved via
> > > > > > gethostname() so the current behaviour does not change. Later, when the
> > > > > > plugin initialization will be moved to provider code, the provider will
> > > > > > decide what hostname will be used (ipa_hostname for example).
> > > > >
> > > > > Rebased on top of latest be_res patches.
> > > >
> > > > When working on IPA plugin I've done some changes to this code. Since it
> > > > is still review pending, I have squashed the changes to this patch set.
> > > > all SRV plugin related things are in a separate header file named
> > > >
> > > > fail_over_srv.h. This way we don't have to #include the whole fail over
> > > > when developing new plugin. The only thing which remained in fail_over.h
> > > > is fo_set_srv_lookup_plugin(), because it should remain hidden for
> > > > providers, see #2
> > > > be_fo_set_srv_lookup_plugin() was added, to keep the fail over opaque
> > > >
> > > > for providers.
> > > > the original sixth patch (set plugin for all providers) was meant to
> > > >
> > > > be reverted, once we agreed on further steps. Now it is completely
> > > > rewritten to final solution on which we agreed with Jakub.
> > > >
> > > > I have the IPA plugin already written (not tested yet), so I believe
> > > > this is the final version if there won't be any review comments.
> > >
> > > //lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > >
> > > OK. One more time :-)
> > > I have slightly enhanced debug messages.
> >
> > I forgot to check if a server is already present in the list. The fifth
> > patch was split in two.
>
> Patch 0001: resolv: add resolv_get_domain request to resolv utils
> Mostly OK, can you just explain this part of the code?
>
> 152     dns_domain = talloc_strdup(state, dns_domain);
> 153     if (dns_domain == NULL) {
> 154         return ENOMEM;
> 155     }
> 156 
> 157     *_dns_domain = talloc_steal(mem_ctx, dns_domain);
> 158 
> 159     return EOK;
>
> Isn't it better to just:
>     *_dns_domain = talloc_strdup(mem_ctx, dns_domain);
> ?
>
> Patch 0002: resolv: add resolv_discover_srv request to resolv utils
> Is it necessary to strdup the input variables? It might be safer in
> case this request outlives the request that created it but in most cases
> it's not really needed. The discovery_domain, on the other hand is
> neither checked not duped.
>
> Patch 0003: DNS sites support - SRV lookup plugin interface
> Code-wise looks good, but I have two questions related to the behaviour.
>
> Why can't the setter overwrite the plugin interface when it's already
> set? Isn't is nice for cases like LDAP where LDAP might set its generic
> plugin first and then IPA which uses LDAP underneath would reset them?
>
> Patch 0004: DNS sites support - SRV DNS lookup plugin
> Please do not use constructs like this unless EOK is expected:
>
> 66 immediately:
> 67     if (ret == EOK) {
> 68         tevent_req_done(req);
> 69     } else {
>
> At the very least it's confusing Coverity and showing up as "dead code".
>
> Same question about strdup-ing input parameters as for patch #2.
>
> I'm not really fond of the name "be_fo_set_standard_srv_lookup_plugin"
> -- standard by what means? What about using the same naming standard as
> in fail_over_srv.c and using be_fo_set_dns_srv_lookup_plugin ?
>
> Also since be_fo_set_standard_srv_lookup_plugin can fail and then the
> failover would not really be usable, it should return errno and users
> should check it.
>
> Patch 0005: fail over - add function to insert multiple servers to the
> list
> Ack
>
> Patch 0006:  DNS sites support - replace SRV lookup code with a plugin
> call
> Did you check if the code can be compiled with this patch? If it can,
> then it's fine that the lookups don't work but I would prefer the
> patches to compile on their own so that git-bisect is usable.
>
> Otherwise ack.
>
> Patch 0007: DNS sites support - use SRV DNS lookup plugin in all providers
> Mostly ack, but when I looked at how the host name is selected in LDAP
> initialization, it seemed to be that this could be moved down the stack
> either to be_fo_set_standard_srv_lookup_plugin or even lower to
> resolv_utils. Anyway, looks ugly in the generic LDAP code :)
>
> Patch 0008: DNS sites support - make fo_discover_srv request visible
> I don't get the point of this patch, why not make the functions pubilc
> from the start?
>
> So far I haven't tested the patches, just read the code to give some
> feedback. I'll reply again when I've tested it.

My testing consisted of positive tests with the IPA (including the one
patch in the other thread) and LDAP backends, those went fine. Negative
tests worked OK, too but revealed one probably uninitialized variable:

[sssd[be[ipaldap]]] [fo_set_port_status] (0x0100): Marking port 0 of server '(no name)' as 'not working'
[sssd[be[ipaldap]]] [resolve_srv_done] (0x0040): Unable to resolve SRV [1432158224]: SRV record not found
                                 ^^^^^^
                                 seems wrong
[sssd[be[ipaldap]]] [set_srv_data_status] (0x0100): Marking SRV lookup of service 'LDAP' as 'not resolved'
[sssd[be[ipaldap]]] [be_resolve_server_process] (0x0080): Couldn't resolve server (SRV lookup meta-server), resolver returned (1432158224)
[sssd[be[ipaldap]]] [be_resolve_server_process] (0x1000): Trying with the next one!

This occured when I queried a server that didn't have the query stored
at all.

The query refresh seemed to worked OK also (I tested by manually
modifiying the timeout). Except for the uninitialized variable, I think
the patches are working good and we just need to resolve the minor code
concerns above.

[Jakub Hrozek]

On Mon, Apr 08, 2013 at 10:38:47PM +0200, Jakub Hrozek wrote:
> On Mon, Apr 08, 2013 at 04:51:47PM +0200, Jakub Hrozek wrote:
> > On Mon, Apr 08, 2013 at 11:55:15AM +0200, Pavel Březina wrote:
> > > On 04/05/2013 01:43 PM, Pavel Březina wrote:
> > > > On 04/05/2013 12:41 PM, Pavel Březina wrote:
> > > > > On 04/03/2013 11:48 AM, Pavel Březina wrote:
> > > > > > On 04/02/2013 03:55 PM, Pavel Březina wrote:
> > > > > > >On 03/27/2013 03:00 PM, Jakub Hrozek wrote:
> > > > > > >>On Wed, Mar 27, 2013 at 11:22:49AM +0100, Pavel Březina wrote:
> > > > > > >>>Later, we can place the plugin architecture inside a module that
> > > > > > >>>knows about SSSD and is placed above resolver. You said you want to
> > > > > > >>>decouple fail over to fail over -> cache -> resolver. This is nice
> > > > > > >>>idea.
> > > > > > >>>
> > > > > > >>>The plugin architecture can be put inside the cache, because cache
> > > > > > >>>is responsible for resolving expired uris and _srv_ and it does not
> > > > > > >>>really say that is has to use resolver directly.
> > > > > > >>
> > > > > > >>OK, this would be the best approach. After more discussion on #sssd
> > > > > > >>with Pavel and Simo we agreed that Pavel will carry on with this
> > > > > > >>approach for the time being.
> > > > > > >>
> > > > > > >>The only two changes in these patches would be better namespacing of
> > > > > > >>the functions used and unifying the resolver initialization with what
> > > > > > >>I'm working on wrt dynamic DNS updates.
> > > > > > >>
> > > > > > >>In the next release, we will create the additional layer and move
> > > > > > >>the plugins there. I moved https://fedorahosted.org/sssd/ticket/1083
> > > > > > >>which is tracking that effort to NEEDS_TRIAGE so we can put it into
> > > > > > >>an appropriate milestone tomorrow.
> > > > > > >
> > > > > > >Hi,
> > > > > > >I'm sending new patch set.
> > > > > > >
> > > > > > >I created async_resolv_utils.c and put there functions for domain
> > > > > > >detection and for resolving SRV with fallback discovery domains.
> > > > > > >This is
> > > > > > >similar to resolve_srv_send and resolve_get_domain_send from the first
> > > > > > >patchset.
> > > > > > >
> > > > > > >resolve_srv_send() was renamed to fo_discover_srv_send() and it wraps
> > > > > > >resolv_discover_srv_send() so that it returns fo_server_info instead of
> > > > > > >ares.
> > > > > > >
> > > > > > >The plugin was renamed to fo_resolve_srv_dns_send(). It is a little bit
> > > > > > >more complicated since it performs domain detection (originally in
> > > > > > >resolve_srv_send()) if the domain is not provided from fail over.
> > > > > > >
> > > > > > >Hostname is passed via plugin context. It is currently retrieved via
> > > > > > >gethostname() so the current behaviour does not change. Later, when the
> > > > > > >plugin initialization will be moved to provider code, the provider will
> > > > > > >decide what hostname will be used (ipa_hostname for example).
> > > > > >
> > > > > > Rebased on top of latest be_res patches.
> > > > >
> > > > > When working on IPA plugin I've done some changes to this code. Since it
> > > > > is still review pending, I have squashed the changes to this patch set.
> > > > > all SRV plugin related things are in a separate header file named
> > > > >
> > > > > fail_over_srv.h. This way we don't have to #include the whole fail over
> > > > > when developing new plugin. The only thing which remained in fail_over.h
> > > > > is fo_set_srv_lookup_plugin(), because it should remain hidden for
> > > > > providers, see #2
> > > > > be_fo_set_srv_lookup_plugin() was added, to keep the fail over opaque
> > > > >
> > > > > for providers.
> > > > > the original sixth patch (set plugin for all providers) was meant to
> > > > >
> > > > > be reverted, once we agreed on further steps. Now it is completely
> > > > > rewritten to final solution on which we agreed with Jakub.
> > > > >
> > > > > I have the IPA plugin already written (not tested yet), so I believe
> > > > > this is the final version if there won't be any review comments.
> > > >
> > > > //lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > >
> > > > OK. One more time :-)
> > > > I have slightly enhanced debug messages.
> > >
> > > I forgot to check if a server is already present in the list. The fifth
> > > patch was split in two.
> >
> > Patch 0001: resolv: add resolv_get_domain request to resolv utils
> > Mostly OK, can you just explain this part of the code?
> >
> > 152     dns_domain = talloc_strdup(state, dns_domain);
> > 153     if (dns_domain == NULL) {
> > 154         return ENOMEM;
> > 155     }
> > 156 
> > 157     *_dns_domain = talloc_steal(mem_ctx, dns_domain);
> > 158 
> > 159     return EOK;
> >
> > Isn't it better to just:
> >     *_dns_domain = talloc_strdup(mem_ctx, dns_domain);
> > ?
> >
> > Patch 0002: resolv: add resolv_discover_srv request to resolv utils
> > Is it necessary to strdup the input variables? It might be safer in
> > case this request outlives the request that created it but in most cases
> > it's not really needed. The discovery_domain, on the other hand is
> > neither checked not duped.
> >
> > Patch 0003: DNS sites support - SRV lookup plugin interface
> > Code-wise looks good, but I have two questions related to the behaviour.
> >
> > Why can't the setter overwrite the plugin interface when it's already
> > set? Isn't is nice for cases like LDAP where LDAP might set its generic
> > plugin first and then IPA which uses LDAP underneath would reset them?
> >
> > Patch 0004: DNS sites support - SRV DNS lookup plugin
> > Please do not use constructs like this unless EOK is expected:
> >
> > 66 immediately:
> > 67     if (ret == EOK) {
> > 68         tevent_req_done(req);
> > 69     } else {
> >
> > At the very least it's confusing Coverity and showing up as "dead code".
> >
> > Same question about strdup-ing input parameters as for patch #2.
> >
> > I'm not really fond of the name "be_fo_set_standard_srv_lookup_plugin"
> > -- standard by what means? What about using the same naming standard as
> > in fail_over_srv.c and using be_fo_set_dns_srv_lookup_plugin ?
> >
> > Also since be_fo_set_standard_srv_lookup_plugin can fail and then the
> > failover would not really be usable, it should return errno and users
> > should check it.
> >
> > Patch 0005: fail over - add function to insert multiple servers to the
> > list
> > Ack
> >
> > Patch 0006:  DNS sites support - replace SRV lookup code with a plugin
> > call
> > Did you check if the code can be compiled with this patch? If it can,
> > then it's fine that the lookups don't work but I would prefer the
> > patches to compile on their own so that git-bisect is usable.
> >
> > Otherwise ack.
> >
> > Patch 0007: DNS sites support - use SRV DNS lookup plugin in all providers
> > Mostly ack, but when I looked at how the host name is selected in LDAP
> > initialization, it seemed to be that this could be moved down the stack
> > either to be_fo_set_standard_srv_lookup_plugin or even lower to
> > resolv_utils. Anyway, looks ugly in the generic LDAP code :)
> >
> > Patch 0008: DNS sites support - make fo_discover_srv request visible
> > I don't get the point of this patch, why not make the functions pubilc
> > from the start?
> >
> > So far I haven't tested the patches, just read the code to give some
> > feedback. I'll reply again when I've tested it.
>
> My testing consisted of positive tests with the IPA (including the one
> patch in the other thread) and LDAP backends, those went fine. Negative
> tests worked OK, too but revealed one probably uninitialized variable:
>
> [sssd[be[ipaldap]]] [fo_set_port_status] (0x0100): Marking port 0 of server '(no name)' as 'not working'
> [sssd[be[ipaldap]]] [resolve_srv_done] (0x0040): Unable to resolve SRV [1432158224]: SRV record not found
>                                  ^^^^^^
>                                  seems wrong
> [sssd[be[ipaldap]]] [set_srv_data_status] (0x0100): Marking SRV lookup of service 'LDAP' as 'not resolved'
> [sssd[be[ipaldap]]] [be_resolve_server_process] (0x0080): Couldn't resolve server (SRV lookup meta-server), resolver returned (1432158224)
> [sssd[be[ipaldap]]] [be_resolve_server_process] (0x1000): Trying with the next one!
>
> This occured when I queried a server that didn't have the query stored
> at all.
>
> The query refresh seemed to worked OK also (I tested by manually
> modifiying the timeout). Except for the uninitialized variable, I think
> the patches are working good and we just need to resolve the minor code
> concerns above.

You also need to put the new header files (src/providers/fail_over_srv.h
and src/providers/ipa/ipa_srv.h I think) into the nodist rule in
Makefile.am

[Pavel Březina]

On 04/08/2013 10:55 PM, Jakub Hrozek wrote:
> On Mon, Apr 08, 2013 at 10:38:47PM +0200, Jakub Hrozek wrote:
> > On Mon, Apr 08, 2013 at 04:51:47PM +0200, Jakub Hrozek wrote:
> > > On Mon, Apr 08, 2013 at 11:55:15AM +0200, Pavel Březina wrote:
> > > > On 04/05/2013 01:43 PM, Pavel Březina wrote:
> > > > > On 04/05/2013 12:41 PM, Pavel Březina wrote:
> > > > > > On 04/03/2013 11:48 AM, Pavel Březina wrote:
> > > > > > > On 04/02/2013 03:55 PM, Pavel Březina wrote:
> > > > > > >> On 03/27/2013 03:00 PM, Jakub Hrozek wrote:
> > > > > > >>> On Wed, Mar 27, 2013 at 11:22:49AM +0100, Pavel Březina wrote:
> > > > > > >>>> Later, we can place the plugin architecture inside a module that
> > > > > > >>>> knows about SSSD and is placed above resolver. You said you want to
> > > > > > >>>> decouple fail over to fail over -> cache -> resolver. This is nice
> > > > > > >>>> idea.
> > > > > > >>>>
> > > > > > >>>> The plugin architecture can be put inside the cache, because cache
> > > > > > >>>> is responsible for resolving expired uris and _srv_ and it does not
> > > > > > >>>> really say that is has to use resolver directly.
> > > > > > >>>
> > > > > > >>> OK, this would be the best approach. After more discussion on #sssd
> > > > > > >>> with Pavel and Simo we agreed that Pavel will carry on with this
> > > > > > >>> approach for the time being.
> > > > > > >>>
> > > > > > >>> The only two changes in these patches would be better namespacing of
> > > > > > >>> the functions used and unifying the resolver initialization with what
> > > > > > >>> I'm working on wrt dynamic DNS updates.
> > > > > > >>>
> > > > > > >>> In the next release, we will create the additional layer and move
> > > > > > >>> the plugins there. I moved https://fedorahosted.org/sssd/ticket/1083
> > > > > > >>> which is tracking that effort to NEEDS_TRIAGE so we can put it into
> > > > > > >>> an appropriate milestone tomorrow.
> > > > > > >>
> > > > > > >> Hi,
> > > > > > >> I'm sending new patch set.
> > > > > > >>
> > > > > > >> I created async_resolv_utils.c and put there functions for domain
> > > > > > >> detection and for resolving SRV with fallback discovery domains.
> > > > > > >> This is
> > > > > > >> similar to resolve_srv_send and resolve_get_domain_send from the first
> > > > > > >> patchset.
> > > > > > >>
> > > > > > >> resolve_srv_send() was renamed to fo_discover_srv_send() and it wraps
> > > > > > >> resolv_discover_srv_send() so that it returns fo_server_info instead of
> > > > > > >> ares.
> > > > > > >>
> > > > > > >> The plugin was renamed to fo_resolve_srv_dns_send(). It is a little bit
> > > > > > >> more complicated since it performs domain detection (originally in
> > > > > > >> resolve_srv_send()) if the domain is not provided from fail over.
> > > > > > >>
> > > > > > >> Hostname is passed via plugin context. It is currently retrieved via
> > > > > > >> gethostname() so the current behaviour does not change. Later, when the
> > > > > > >> plugin initialization will be moved to provider code, the provider will
> > > > > > >> decide what hostname will be used (ipa_hostname for example).
> > > > > > >
> > > > > > > Rebased on top of latest be_res patches.
> > > > > >
> > > > > > When working on IPA plugin I've done some changes to this code. Since it
> > > > > > is still review pending, I have squashed the changes to this patch set.
> > > > > > all SRV plugin related things are in a separate header file named
> > > > > >
> > > > > > fail_over_srv.h. This way we don't have to #include the whole fail over
> > > > > > when developing new plugin. The only thing which remained in fail_over.h
> > > > > > is fo_set_srv_lookup_plugin(), because it should remain hidden for
> > > > > > providers, see #2
> > > > > > be_fo_set_srv_lookup_plugin() was added, to keep the fail over opaque
> > > > > >
> > > > > > for providers.
> > > > > > the original sixth patch (set plugin for all providers) was meant to
> > > > > >
> > > > > > be reverted, once we agreed on further steps. Now it is completely
> > > > > > rewritten to final solution on which we agreed with Jakub.
> > > > > >
> > > > > > I have the IPA plugin already written (not tested yet), so I believe
> > > > > > this is the final version if there won't be any review comments.
> > > > >
> > > > > //lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > >
> > > > > OK. One more time :-)
> > > > > I have slightly enhanced debug messages.
> > > >
> > > > I forgot to check if a server is already present in the list. The fifth
> > > > patch was split in two.

Segfault fixed.
> > > Patch 0001: resolv: add resolv_get_domain request to resolv utils
> > > Mostly OK, can you just explain this part of the code?
> > >
> > > 152     dns_domain = talloc_strdup(state, dns_domain);
> > > 153     if (dns_domain == NULL) {
> > > 154         return ENOMEM;
> > > 155     }
> > > 156
> > > 157     *_dns_domain = talloc_steal(mem_ctx, dns_domain);
> > > 158
> > > 159     return EOK;
> > >
> > > Isn't it better to just:
> > >      *_dns_domain = talloc_strdup(mem_ctx, dns_domain);
> > > ?

I think it is better to not touch output parameters at all in case of 
failure. But I don't feel so strong about it. Done.
> > > Patch 0002: resolv: add resolv_discover_srv request to resolv utils
> > > Is it necessary to strdup the input variables? It might be safer in
> > > case this request outlives the request that created it but in most cases
> > > it's not really needed. The discovery_domain, on the other hand is
> > > neither checked not duped.

I remember fixing similar issue in resolver some time ago. So that
probably made me write more robust code here.
58b335985e75672e4de699351ab1182cbd7aa990

But you are right that it is unlikely to happen here.
Removed strdup() and added check for discovery_domain.
> > > Patch 0003: DNS sites support - SRV lookup plugin interface
> > > Code-wise looks good, but I have two questions related to the behaviour.
> > >
> > > Why can't the setter overwrite the plugin interface when it's already
> > > set? Isn't is nice for cases like LDAP where LDAP might set its generic
> > > plugin first and then IPA which uses LDAP underneath would reset them?

We agreed that id_provider is authoritative for plugin choise. And 
id_provider is initialized at first. Therefore it is not desirable to 
override it. It is described in #0007 commit message, but maybe I should 
put a comment here as well.
> > > Patch 0004: DNS sites support - SRV DNS lookup plugin
> > > Please do not use constructs like this unless EOK is expected:
> > >
> > > 66 immediately:
> > > 67     if (ret == EOK) {
> > > 68         tevent_req_done(req);
> > > 69     } else {
> > >
> > > At the very least it's confusing Coverity and showing up as "dead code".

Done.
> > > Same question about strdup-ing input parameters as for patch #2.

Done.
> > > I'm not really fond of the name "be_fo_set_standard_srv_lookup_plugin"
> > > -- standard by what means? What about using the same naming standard as
> > > in fail_over_srv.c and using be_fo_set_dns_srv_lookup_plugin ?

Done.
> > > Also since be_fo_set_standard_srv_lookup_plugin can fail and then the
> > > failover would not really be usable, it should return errno and users
> > > should check it.

Done.
> > > Patch 0005: fail over - add function to insert multiple servers to the
> > > list
> > > Ack
> > >
> > > Patch 0006:  DNS sites support - replace SRV lookup code with a plugin
> > > call
> > > Did you check if the code can be compiled with this patch? If it can,
> > > then it's fine that the lookups don't work but I would prefer the
> > > patches to compile on their own so that git-bisect is usable.

Yes, it compiles fine.
> > > Otherwise ack.
> > >
> > > Patch 0007: DNS sites support - use SRV DNS lookup plugin in all providers
> > > Mostly ack, but when I looked at how the host name is selected in LDAP
> > > initialization, it seemed to be that this could be moved down the stack
> > > either to be_fo_set_standard_srv_lookup_plugin or even lower to
> > > resolv_utils. Anyway, looks ugly in the generic LDAP code :)

Moved to be_fo_set_dns_srv_lookup_plugin().
> > > Patch 0008: DNS sites support - make fo_discover_srv request visible
> > > I don't get the point of this patch, why not make the functions pubilc
> > > from the start?

Darn, this belonged to the IPA plugin patch. But yes, it can be also 
squashed in the forth patch.
> > > So far I haven't tested the patches, just read the code to give some
> > > feedback. I'll reply again when I've tested it.
> >
> > My testing consisted of positive tests with the IPA (including the one
> > patch in the other thread) and LDAP backends, those went fine. Negative
> > tests worked OK, too but revealed one probably uninitialized variable:
> >
> > [sssd[be[ipaldap]]] [fo_set_port_status] (0x0100): Marking port 0 of server '(no name)' as 'not working'
> > [sssd[be[ipaldap]]] [resolve_srv_done] (0x0040): Unable to resolve SRV [1432158224]: SRV record not found
> >                                   ^^^^^^
> >                                   seems wrong
> > [sssd[be[ipaldap]]] [set_srv_data_status] (0x0100): Marking SRV lookup of service 'LDAP' as 'not resolved'
> > [sssd[be[ipaldap]]] [be_resolve_server_process] (0x0080): Couldn't resolve server (SRV lookup meta-server), resolver returned (1432158224)
> > [sssd[be[ipaldap]]] [be_resolve_server_process] (0x1000): Trying with the next one!
> >
> > This occured when I queried a server that didn't have the query stored
> > at all.
> >
> > The query refresh seemed to worked OK also (I tested by manually
> > modifiying the timeout). Except for the uninitialized variable, I think
> > the patches are working good and we just need to resolve the minor code
> > concerns above.
>
> You also need to put the new header files (src/providers/fail_over_srv.h
> and src/providers/ipa/ipa_srv.h I think) into the nodist rule in
> Makefile.am

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

[Jakub Hrozek]

On Tue, Apr 09, 2013 at 02:35:24PM +0200, Pavel Březina wrote:
> > > > Patch 0001: resolv: add resolv_get_domain request to resolv utils
> > > > Mostly OK, can you just explain this part of the code?
> > > >
> > > > 152     dns_domain = talloc_strdup(state, dns_domain);
> > > > 153     if (dns_domain == NULL) {
> > > > 154         return ENOMEM;
> > > > 155     }
> > > > 156
> > > > 157     *_dns_domain = talloc_steal(mem_ctx, dns_domain);
> > > > 158
> > > > 159     return EOK;
> > > >
> > > > Isn't it better to just:
> > > >     *_dns_domain = talloc_strdup(mem_ctx, dns_domain);
> > > > ?
>
> I think it is better to not touch output parameters at all in case
> of failure. But I don't feel so strong about it. Done.

You still need to check the result of strdup for NULL :)

Otherwise Ack.
> > > > Patch 0002: resolv: add resolv_discover_srv request to resolv utils
> > > > Is it necessary to strdup the input variables? It might be safer in
> > > > case this request outlives the request that created it but in most cases
> > > > it's not really needed. The discovery_domain, on the other hand is
> > > > neither checked not duped.
>
> I remember fixing similar issue in resolver some time ago. So that
> probably made me write more robust code here.
> 58b335985e75672e4de699351ab1182cbd7aa990
>
> But you are right that it is unlikely to happen here.
> Removed strdup() and added check for discovery_domain.

I was actually thinking about it a bit and I think that there should be
no guesswork in an API that is provided for other modules. The API
should be able to survive its parent request going away.

No need to revert this part now, I will file a ticket and we can do this
minor change post-beta.

Sorry I changed my mind after asking you to change the code.

Ack for now.
> > > > Patch 0003: DNS sites support - SRV lookup plugin interface
> > > > Code-wise looks good, but I have two questions related to the behaviour.
> > > >
> > > > Why can't the setter overwrite the plugin interface when it's already
> > > > set? Isn't is nice for cases like LDAP where LDAP might set its generic
> > > > plugin first and then IPA which uses LDAP underneath would reset them?
>
> We agreed that id_provider is authoritative for plugin choise. And
> id_provider is initialized at first. Therefore it is not desirable
> to override it. It is described in #0007 commit message, but maybe I
> should put a comment here as well.

You're right, correct.
> > > > Patch 0004: DNS sites support - SRV DNS lookup plugin
> > > > Please do not use constructs like this unless EOK is expected:
> > > >
> > > > 66 immediately:
> > > > 67     if (ret == EOK) {
> > > > 68         tevent_req_done(req);
> > > > 69     } else {
> > > >
> > > > At the very least it's confusing Coverity and showing up as "dead code".
>
> Done.

There is still one part like this in patch #1, can you remove it too
when you fix the strdup?
> > > > Same question about strdup-ing input parameters as for patch #2.
>
> Done.
> > > > I'm not really fond of the name "be_fo_set_standard_srv_lookup_plugin"
> > > > -- standard by what means? What about using the same naming standard as
> > > > in fail_over_srv.c and using be_fo_set_dns_srv_lookup_plugin ?
>
> Done.
> > > > Also since be_fo_set_standard_srv_lookup_plugin can fail and then the
> > > > failover would not really be usable, it should return errno and users
> > > > should check it.
>
> Done.
> > > > Patch 0005: fail over - add function to insert multiple servers to the
> > > > list
> > > > Ack

Still ack.
> > > > Patch 0006:  DNS sites support - replace SRV lookup code with a plugin
> > > > call
> > > > Did you check if the code can be compiled with this patch? If it can,
> > > > then it's fine that the lookups don't work but I would prefer the
> > > > patches to compile on their own so that git-bisect is usable.
>
> Yes, it compiles fine.

Ack
> > > > Otherwise ack.
> > > >
> > > > Patch 0007: DNS sites support - use SRV DNS lookup plugin in all providers
> > > > Mostly ack, but when I looked at how the host name is selected in LDAP
> > > > initialization, it seemed to be that this could be moved down the stack
> > > > either to be_fo_set_standard_srv_lookup_plugin or even lower to
> > > > resolv_utils. Anyway, looks ugly in the generic LDAP code :)
>
> Moved to be_fo_set_dns_srv_lookup_plugin().

Ack
> > > > Patch 0008: DNS sites support - make fo_discover_srv request visible
> > > > I don't get the point of this patch, why not make the functions pubilc
> > > > from the start?
>
> Darn, this belonged to the IPA plugin patch. But yes, it can be also
> squashed in the forth patch.
> > > > So far I haven't tested the patches, just read the code to give some
> > > > feedback. I'll reply again when I've tested it.
> > >
> > > My testing consisted of positive tests with the IPA (including the one
> > > patch in the other thread) and LDAP backends, those went fine. Negative
> > > tests worked OK, too but revealed one probably uninitialized variable:
> > >
> > > [sssd[be[ipaldap]]] [fo_set_port_status] (0x0100): Marking port 0 of server '(no name)' as 'not working'
> > > [sssd[be[ipaldap]]] [resolve_srv_done] (0x0040): Unable to resolve SRV [1432158224]: SRV record not found
> > >                                  ^^^^^^
> > >                                  seems wrong
> > > [sssd[be[ipaldap]]] [set_srv_data_status] (0x0100): Marking SRV lookup of service 'LDAP' as 'not resolved'
> > > [sssd[be[ipaldap]]] [be_resolve_server_process] (0x0080): Couldn't resolve server (SRV lookup meta-server), resolver returned (1432158224)
> > > [sssd[be[ipaldap]]] [be_resolve_server_process] (0x1000): Trying with the next one!
> > >
> > > This occured when I queried a server that didn't have the query stored
> > > at all.
> > >
> > > The query refresh seemed to worked OK also (I tested by manually
> > > modifiying the timeout). Except for the uninitialized variable, I think
> > > the patches are working good and we just need to resolve the minor code
> > > concerns above.
> >
> > You also need to put the new header files (src/providers/fail_over_srv.h
> > and src/providers/ipa/ipa_srv.h I think) into the nodist rule in
> > Makefile.am
>
> Done.

I couldn't compile these patches without the IPA discovery patch on top,
I was getting:
/home/remote/jhrozek/devel/sssd/src/providers/ipa/ipa_init.c: In
function 'sssm_ipa_id_init':
/home/remote/jhrozek/devel/sssd/src/providers/ipa/ipa_init.c:213:5:
error: implicit declaration of function
'be_fo_set_standard_srv_lookup_plugin'
[-Werror=implicit-function-declaration]

Probably some hunk got squashed into the IPA discovery patch?

Simple testing with all these patches and the IPA one on top went fine.

[Pavel Březina]

On 04/10/2013 01:28 PM, Jakub Hrozek wrote:
> On Tue, Apr 09, 2013 at 02:35:24PM +0200, Pavel Březina wrote:
> > > > > Patch 0001: resolv: add resolv_get_domain request to resolv utils
> > > > > Mostly OK, can you just explain this part of the code?
> > > > >
> > > > > 152     dns_domain = talloc_strdup(state, dns_domain);
> > > > > 153     if (dns_domain == NULL) {
> > > > > 154         return ENOMEM;
> > > > > 155     }
> > > > > 156
> > > > > 157     *_dns_domain = talloc_steal(mem_ctx, dns_domain);
> > > > > 158
> > > > > 159     return EOK;
> > > > >
> > > > > Isn't it better to just:
> > > > >      *_dns_domain = talloc_strdup(mem_ctx, dns_domain);
> > > > > ?
> >
> > I think it is better to not touch output parameters at all in case
> > of failure. But I don't feel so strong about it. Done.
>
> You still need to check the result of strdup for NULL :)

Right, thanks.
> Otherwise Ack.
> > > > > Patch 0002: resolv: add resolv_discover_srv request to resolv utils
> > > > > Is it necessary to strdup the input variables? It might be safer in
> > > > > case this request outlives the request that created it but in most cases
> > > > > it's not really needed. The discovery_domain, on the other hand is
> > > > > neither checked not duped.
> >
> > I remember fixing similar issue in resolver some time ago. So that
> > probably made me write more robust code here.
> > 58b335985e75672e4de699351ab1182cbd7aa990
> >
> > But you are right that it is unlikely to happen here.
> > Removed strdup() and added check for discovery_domain.
>
> I was actually thinking about it a bit and I think that there should be
> no guesswork in an API that is provided for other modules. The API
> should be able to survive its parent request going away.
>
> No need to revert this part now, I will file a ticket and we can do this
> minor change post-beta.
>
> Sorry I changed my mind after asking you to change the code.
>
> Ack for now.
> > > > > Patch 0003: DNS sites support - SRV lookup plugin interface
> > > > > Code-wise looks good, but I have two questions related to the behaviour.
> > > > >
> > > > > Why can't the setter overwrite the plugin interface when it's already
> > > > > set? Isn't is nice for cases like LDAP where LDAP might set its generic
> > > > > plugin first and then IPA which uses LDAP underneath would reset them?
> >
> > We agreed that id_provider is authoritative for plugin choise. And
> > id_provider is initialized at first. Therefore it is not desirable
> > to override it. It is described in #0007 commit message, but maybe I
> > should put a comment here as well.
>
> You're right, correct.
> > > > > Patch 0004: DNS sites support - SRV DNS lookup plugin
> > > > > Please do not use constructs like this unless EOK is expected:
> > > > >
> > > > > 66 immediately:
> > > > > 67     if (ret == EOK) {
> > > > > 68         tevent_req_done(req);
> > > > > 69     } else {
> > > > >
> > > > > At the very least it's confusing Coverity and showing up as "dead code".
> >
> > Done.
>
> There is still one part like this in patch #1, can you remove it too

Done.
> when you fix the strdup?
> > > > > Same question about strdup-ing input parameters as for patch #2.
> >
> > Done.
> > > > > I'm not really fond of the name "be_fo_set_standard_srv_lookup_plugin"
> > > > > -- standard by what means? What about using the same naming standard as
> > > > > in fail_over_srv.c and using be_fo_set_dns_srv_lookup_plugin ?
> >
> > Done.
> > > > > Also since be_fo_set_standard_srv_lookup_plugin can fail and then the
> > > > > failover would not really be usable, it should return errno and users
> > > > > should check it.
> >
> > Done.
> > > > > Patch 0005: fail over - add function to insert multiple servers to the
> > > > > list
> > > > > Ack
>
> Still ack.
> > > > > Patch 0006:  DNS sites support - replace SRV lookup code with a plugin
> > > > > call
> > > > > Did you check if the code can be compiled with this patch? If it can,
> > > > > then it's fine that the lookups don't work but I would prefer the
> > > > > patches to compile on their own so that git-bisect is usable.
> >
> > Yes, it compiles fine.
>
> Ack
> > > > > Otherwise ack.
> > > > >
> > > > > Patch 0007: DNS sites support - use SRV DNS lookup plugin in all providers
> > > > > Mostly ack, but when I looked at how the host name is selected in LDAP
> > > > > initialization, it seemed to be that this could be moved down the stack
> > > > > either to be_fo_set_standard_srv_lookup_plugin or even lower to
> > > > > resolv_utils. Anyway, looks ugly in the generic LDAP code :)
> >
> > Moved to be_fo_set_dns_srv_lookup_plugin().
>
> Ack
> > > > > Patch 0008: DNS sites support - make fo_discover_srv request visible
> > > > > I don't get the point of this patch, why not make the functions pubilc
> > > > > from the start?
> >
> > Darn, this belonged to the IPA plugin patch. But yes, it can be also
> > squashed in the forth patch.
> > > > > So far I haven't tested the patches, just read the code to give some
> > > > > feedback. I'll reply again when I've tested it.
> > > >
> > > > My testing consisted of positive tests with the IPA (including the one
> > > > patch in the other thread) and LDAP backends, those went fine. Negative
> > > > tests worked OK, too but revealed one probably uninitialized variable:
> > > >
> > > > [sssd[be[ipaldap]]] [fo_set_port_status] (0x0100): Marking port 0 of server '(no name)' as 'not working'
> > > > [sssd[be[ipaldap]]] [resolve_srv_done] (0x0040): Unable to resolve SRV [1432158224]: SRV record not found
> > > >                                   ^^^^^^
> > > >                                   seems wrong
> > > > [sssd[be[ipaldap]]] [set_srv_data_status] (0x0100): Marking SRV lookup of service 'LDAP' as 'not resolved'
> > > > [sssd[be[ipaldap]]] [be_resolve_server_process] (0x0080): Couldn't resolve server (SRV lookup meta-server), resolver returned (1432158224)
> > > > [sssd[be[ipaldap]]] [be_resolve_server_process] (0x1000): Trying with the next one!
> > > >
> > > > This occured when I queried a server that didn't have the query stored
> > > > at all.
> > > >
> > > > The query refresh seemed to worked OK also (I tested by manually
> > > > modifiying the timeout). Except for the uninitialized variable, I think
> > > > the patches are working good and we just need to resolve the minor code
> > > > concerns above.
> > >
> > > You also need to put the new header files (src/providers/fail_over_srv.h
> > > and src/providers/ipa/ipa_srv.h I think) into the nodist rule in
> > > Makefile.am
> >
> > Done.
>
> I couldn't compile these patches without the IPA discovery patch on top,
> I was getting:
> /home/remote/jhrozek/devel/sssd/src/providers/ipa/ipa_init.c: In
> function 'sssm_ipa_id_init':
> /home/remote/jhrozek/devel/sssd/src/providers/ipa/ipa_init.c:213:5:
> error: implicit declaration of function
> 'be_fo_set_standard_srv_lookup_plugin'
> [-Werror=implicit-function-declaration]
>
> Probably some hunk got squashed into the IPA discovery patch?

You are correct. Fixed.
> Simple testing with all these patches and the IPA one on top went fine.
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Wed, Apr 10, 2013 at 02:22:26PM +0200, Pavel Březina wrote:
> On 04/10/2013 01:28 PM, Jakub Hrozek wrote:
> > On Tue, Apr 09, 2013 at 02:35:24PM +0200, Pavel Březina wrote:
> > > > > > Patch 0001: resolv: add resolv_get_domain request to resolv utils
> > > > > > Mostly OK, can you just explain this part of the code?
> > > > > >
> > > > > > 152     dns_domain = talloc_strdup(state, dns_domain);
> > > > > > 153     if (dns_domain == NULL) {
> > > > > > 154         return ENOMEM;
> > > > > > 155     }
> > > > > > 156
> > > > > > 157     *_dns_domain = talloc_steal(mem_ctx, dns_domain);
> > > > > > 158
> > > > > > 159     return EOK;
> > > > > >
> > > > > > Isn't it better to just:
> > > > > >     *_dns_domain = talloc_strdup(mem_ctx, dns_domain);
> > > > > > ?
> > >
> > > I think it is better to not touch output parameters at all in case
> > > of failure. But I don't feel so strong about it. Done.
> >
> > You still need to check the result of strdup for NULL :)
>
> Right, thanks.

Ack
> > Otherwise Ack.
> > > > > > Patch 0002: resolv: add resolv_discover_srv request to resolv utils
> > > > > > Is it necessary to strdup the input variables? It might be safer in
> > > > > > case this request outlives the request that created it but in most cases
> > > > > > it's not really needed. The discovery_domain, on the other hand is
> > > > > > neither checked not duped.
> > >
> > > I remember fixing similar issue in resolver some time ago. So that
> > > probably made me write more robust code here.
> > > 58b335985e75672e4de699351ab1182cbd7aa990
> > >
> > > But you are right that it is unlikely to happen here.
> > > Removed strdup() and added check for discovery_domain.
> >
> > I was actually thinking about it a bit and I think that there should be
> > no guesswork in an API that is provided for other modules. The API
> > should be able to survive its parent request going away.
> >
> > No need to revert this part now, I will file a ticket and we can do this
> > minor change post-beta.

https://fedorahosted.org/sssd/ticket/1875
> > Sorry I changed my mind after asking you to change the code.
> >
> > Ack for now.

Ack
> > > > > > Patch 0003: DNS sites support - SRV lookup plugin interface
> > > > > > Code-wise looks good, but I have two questions related to the behaviour.
> > > > > >
> > > > > > Why can't the setter overwrite the plugin interface when it's already
> > > > > > set? Isn't is nice for cases like LDAP where LDAP might set its generic
> > > > > > plugin first and then IPA which uses LDAP underneath would reset them?
> > >
> > > We agreed that id_provider is authoritative for plugin choise. And
> > > id_provider is initialized at first. Therefore it is not desirable
> > > to override it. It is described in #0007 commit message, but maybe I
> > > should put a comment here as well.
> >
> > You're right, correct.
> > > > > > Patch 0004: DNS sites support - SRV DNS lookup plugin
> > > > > > Please do not use constructs like this unless EOK is expected:
> > > > > >
> > > > > > 66 immediately:
> > > > > > 67     if (ret == EOK) {
> > > > > > 68         tevent_req_done(req);
> > > > > > 69     } else {
> > > > > >
> > > > > > At the very least it's confusing Coverity and showing up as "dead code".
> > >
> > > Done.
> >
> > There is still one part like this in patch #1, can you remove it too
>
> Done.
> > when you fix the strdup?

Ack
> > > > > > Same question about strdup-ing input parameters as for patch #2.
> > >
> > > Done.
> > > > > > I'm not really fond of the name "be_fo_set_standard_srv_lookup_plugin"
> > > > > > -- standard by what means? What about using the same naming standard as
> > > > > > in fail_over_srv.c and using be_fo_set_dns_srv_lookup_plugin ?
> > >
> > > Done.
> > > > > > Also since be_fo_set_standard_srv_lookup_plugin can fail and then the
> > > > > > failover would not really be usable, it should return errno and users
> > > > > > should check it.
> > >
> > > Done.
> > > > > > Patch 0005: fail over - add function to insert multiple servers to the
> > > > > > list
> > > > > > Ack
> >
> > Still ack.
> > > > > > Patch 0006:  DNS sites support - replace SRV lookup code with a plugin
> > > > > > call
> > > > > > Did you check if the code can be compiled with this patch? If it can,
> > > > > > then it's fine that the lookups don't work but I would prefer the
> > > > > > patches to compile on their own so that git-bisect is usable.
> > >
> > > Yes, it compiles fine.
> >
> > Ack
> > > > > > Otherwise ack.
> > > > > >
> > > > > > Patch 0007: DNS sites support - use SRV DNS lookup plugin in all providers
> > > > > > Mostly ack, but when I looked at how the host name is selected in LDAP
> > > > > > initialization, it seemed to be that this could be moved down the stack
> > > > > > either to be_fo_set_standard_srv_lookup_plugin or even lower to
> > > > > > resolv_utils. Anyway, looks ugly in the generic LDAP code :)
> > >
> > > Moved to be_fo_set_dns_srv_lookup_plugin().
> >
> > Ack
> > > > > > Patch 0008: DNS sites support - make fo_discover_srv request visible
> > > > > > I don't get the point of this patch, why not make the functions pubilc
> > > > > > from the start?
> > >
> > > Darn, this belonged to the IPA plugin patch. But yes, it can be also
> > > squashed in the forth patch.
> > > > > > So far I haven't tested the patches, just read the code to give some
> > > > > > feedback. I'll reply again when I've tested it.
> > > > >
> > > > > My testing consisted of positive tests with the IPA (including the one
> > > > > patch in the other thread) and LDAP backends, those went fine. Negative
> > > > > tests worked OK, too but revealed one probably uninitialized variable:
> > > > >
> > > > > [sssd[be[ipaldap]]] [fo_set_port_status] (0x0100): Marking port 0 of server '(no name)' as 'not working'
> > > > > [sssd[be[ipaldap]]] [resolve_srv_done] (0x0040): Unable to resolve SRV [1432158224]: SRV record not found
> > > > >                                  ^^^^^^
> > > > >                                  seems wrong
> > > > > [sssd[be[ipaldap]]] [set_srv_data_status] (0x0100): Marking SRV lookup of service 'LDAP' as 'not resolved'
> > > > > [sssd[be[ipaldap]]] [be_resolve_server_process] (0x0080): Couldn't resolve server (SRV lookup meta-server), resolver returned (1432158224)
> > > > > [sssd[be[ipaldap]]] [be_resolve_server_process] (0x1000): Trying with the next one!
> > > > >
> > > > > This occured when I queried a server that didn't have the query stored
> > > > > at all.
> > > > >
> > > > > The query refresh seemed to worked OK also (I tested by manually
> > > > > modifiying the timeout). Except for the uninitialized variable, I think
> > > > > the patches are working good and we just need to resolve the minor code
> > > > > concerns above.
> > > >
> > > > You also need to put the new header files (src/providers/fail_over_srv.h
> > > > and src/providers/ipa/ipa_srv.h I think) into the nodist rule in
> > > > Makefile.am
> > >
> > > Done.
> >
> > I couldn't compile these patches without the IPA discovery patch on top,
> > I was getting:
> > /home/remote/jhrozek/devel/sssd/src/providers/ipa/ipa_init.c: In
> > function 'sssm_ipa_id_init':
> > /home/remote/jhrozek/devel/sssd/src/providers/ipa/ipa_init.c:213:5:
> > error: implicit declaration of function
> > 'be_fo_set_standard_srv_lookup_plugin'
> > [-Werror=implicit-function-declaration]
> >
> > Probably some hunk got squashed into the IPA discovery patch?
>
> You are correct. Fixed.

Ack
> > Simple testing with all these patches and the IPA one on top went fine.

Simple SRV testing also passed with this round of patches.

All patches are acked now.

[Jakub Hrozek]

On Wed, Apr 10, 2013 at 03:33:37PM +0200, Jakub Hrozek wrote:
> Simple SRV testing also passed with this round of patches.
>
> All patches are acked now.

Pushed all patches to master.

[Pavel Březina]

----- Original Message -----
> From: "Jakub Hrozek" 
> To: [e-mail]
> Sent: Monday, April 8, 2013 10:38:47 PM
> Subject: Re: [SSSD] [PATCH] DNS site support 1st wave - generic SRV lookup plugin
>
> On Mon, Apr 08, 2013 at 04:51:47PM +0200, Jakub Hrozek wrote:
> > On Mon, Apr 08, 2013 at 11:55:15AM +0200, Pavel Březina wrote:
> > > On 04/05/2013 01:43 PM, Pavel Březina wrote:
> > > > On 04/05/2013 12:41 PM, Pavel Březina wrote:
> > > > > On 04/03/2013 11:48 AM, Pavel Březina wrote:
> > > > > > On 04/02/2013 03:55 PM, Pavel Březina wrote:
> > > > > > >On 03/27/2013 03:00 PM, Jakub Hrozek wrote:
> > > > > > >>On Wed, Mar 27, 2013 at 11:22:49AM +0100, Pavel Březina wrote:
> > > > > > >>>Later, we can place the plugin architecture inside a module that
> > > > > > >>>knows about SSSD and is placed above resolver. You said you want to
> > > > > > >>>decouple fail over to fail over -> cache -> resolver. This is nice
> > > > > > >>>idea.
> > > > > > >>>
> > > > > > >>>The plugin architecture can be put inside the cache, because cache
> > > > > > >>>is responsible for resolving expired uris and _srv_ and it does not
> > > > > > >>>really say that is has to use resolver directly.
> > > > > > >>
> > > > > > >>OK, this would be the best approach. After more discussion on #sssd
> > > > > > >>with Pavel and Simo we agreed that Pavel will carry on with this
> > > > > > >>approach for the time being.
> > > > > > >>
> > > > > > >>The only two changes in these patches would be better namespacing of
> > > > > > >>the functions used and unifying the resolver initialization with
> > > > > > >>what
> > > > > > >>I'm working on wrt dynamic DNS updates.
> > > > > > >>
> > > > > > >>In the next release, we will create the additional layer and move
> > > > > > >>the plugins there. I moved https://fedorahosted.org/sssd/ticket/1083
> > > > > > >>which is tracking that effort to NEEDS_TRIAGE so we can put it into
> > > > > > >>an appropriate milestone tomorrow.
> > > > > > >
> > > > > > >Hi,
> > > > > > >I'm sending new patch set.
> > > > > > >
> > > > > > >I created async_resolv_utils.c and put there functions for domain
> > > > > > >detection and for resolving SRV with fallback discovery domains.
> > > > > > >This is
> > > > > > >similar to resolve_srv_send and resolve_get_domain_send from the
> > > > > > >first
> > > > > > >patchset.
> > > > > > >
> > > > > > >resolve_srv_send() was renamed to fo_discover_srv_send() and it wraps
> > > > > > >resolv_discover_srv_send() so that it returns fo_server_info instead
> > > > > > >of
> > > > > > >ares.
> > > > > > >
> > > > > > >The plugin was renamed to fo_resolve_srv_dns_send(). It is a little
> > > > > > >bit
> > > > > > >more complicated since it performs domain detection (originally in
> > > > > > >resolve_srv_send()) if the domain is not provided from fail over.
> > > > > > >
> > > > > > >Hostname is passed via plugin context. It is currently retrieved via
> > > > > > >gethostname() so the current behaviour does not change. Later, when
> > > > > > >the
> > > > > > >plugin initialization will be moved to provider code, the provider
> > > > > > >will
> > > > > > >decide what hostname will be used (ipa_hostname for example).
> > > > > >
> > > > > > Rebased on top of latest be_res patches.
> > > > >
> > > > > When working on IPA plugin I've done some changes to this code. Since
> > > > > it
> > > > > is still review pending, I have squashed the changes to this patch set.
> > > > > all SRV plugin related things are in a separate header file named
> > > > >
> > > > > fail_over_srv.h. This way we don't have to #include the whole fail over
> > > > > when developing new plugin. The only thing which remained in
> > > > > fail_over.h
> > > > > is fo_set_srv_lookup_plugin(), because it should remain hidden for
> > > > > providers, see #2
> > > > > be_fo_set_srv_lookup_plugin() was added, to keep the fail over
> > > > >
> > > > > opaque
> > > > > for providers.
> > > > > the original sixth patch (set plugin for all providers) was meant to
> > > > >
> > > > > be reverted, once we agreed on further steps. Now it is completely
> > > > > rewritten to final solution on which we agreed with Jakub.
> > > > >
> > > > > I have the IPA plugin already written (not tested yet), so I believe
> > > > > this is the final version if there won't be any review comments.
> > > >
> > > > //lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > >
> > > > OK. One more time :-)
> > > > I have slightly enhanced debug messages.
> > >
> > > I forgot to check if a server is already present in the list. The fifth
> > > patch was split in two.
> >
> > Patch 0001: resolv: add resolv_get_domain request to resolv utils
> > Mostly OK, can you just explain this part of the code?
> >
> > 152     dns_domain = talloc_strdup(state, dns_domain);
> > 153     if (dns_domain == NULL) {
> > 154         return ENOMEM;
> > 155     }
> > 156
> > 157     *_dns_domain = talloc_steal(mem_ctx, dns_domain);
> > 158
> > 159     return EOK;
> >
> > Isn't it better to just:
> >     *_dns_domain = talloc_strdup(mem_ctx, dns_domain);
> > ?
> >
> > Patch 0002: resolv: add resolv_discover_srv request to resolv utils
> > Is it necessary to strdup the input variables? It might be safer in
> > case this request outlives the request that created it but in most cases
> > it's not really needed. The discovery_domain, on the other hand is
> > neither checked not duped.
> >
> > Patch 0003: DNS sites support - SRV lookup plugin interface
> > Code-wise looks good, but I have two questions related to the behaviour.
> >
> > Why can't the setter overwrite the plugin interface when it's already
> > set? Isn't is nice for cases like LDAP where LDAP might set its generic
> > plugin first and then IPA which uses LDAP underneath would reset them?
> >
> > Patch 0004: DNS sites support - SRV DNS lookup plugin
> > Please do not use constructs like this unless EOK is expected:
> >
> > 66 immediately:
> > 67     if (ret == EOK) {
> > 68         tevent_req_done(req);
> > 69     } else {
> >
> > At the very least it's confusing Coverity and showing up as "dead code".
> >
> > Same question about strdup-ing input parameters as for patch #2.
> >
> > I'm not really fond of the name "be_fo_set_standard_srv_lookup_plugin"
> > -- standard by what means? What about using the same naming standard as
> > in fail_over_srv.c and using be_fo_set_dns_srv_lookup_plugin ?
> >
> > Also since be_fo_set_standard_srv_lookup_plugin can fail and then the
> > failover would not really be usable, it should return errno and users
> > should check it.
> >
> > Patch 0005: fail over - add function to insert multiple servers to the
> > list
> > Ack
> >
> > Patch 0006:  DNS sites support - replace SRV lookup code with a plugin
> > call
> > Did you check if the code can be compiled with this patch? If it can,
> > then it's fine that the lookups don't work but I would prefer the
> > patches to compile on their own so that git-bisect is usable.
> >
> > Otherwise ack.
> >
> > Patch 0007: DNS sites support - use SRV DNS lookup plugin in all providers
> > Mostly ack, but when I looked at how the host name is selected in LDAP
> > initialization, it seemed to be that this could be moved down the stack
> > either to be_fo_set_standard_srv_lookup_plugin or even lower to
> > resolv_utils. Anyway, looks ugly in the generic LDAP code :)
> >
> > Patch 0008: DNS sites support - make fo_discover_srv request visible
> > I don't get the point of this patch, why not make the functions pubilc
> > from the start?
> >
> > So far I haven't tested the patches, just read the code to give some
> > feedback. I'll reply again when I've tested it.
>
> My testing consisted of positive tests with the IPA (including the one
> patch in the other thread) and LDAP backends, those went fine. Negative
> tests worked OK, too but revealed one probably uninitialized variable:
>
> [sssd[be[ipaldap]]] [fo_set_port_status] (0x0100): Marking port 0 of server
> '(no name)' as 'not working'
> [sssd[be[ipaldap]]] [resolve_srv_done] (0x0040): Unable to resolve SRV
> [1432158224]: SRV record not found
>                                  ^^^^^^
>                                  seems wrong
> [sssd[be[ipaldap]]] [set_srv_data_status] (0x0100): Marking SRV lookup of
> service 'LDAP' as 'not resolved'
> [sssd[be[ipaldap]]] [be_resolve_server_process] (0x0080): Couldn't resolve
> server (SRV lookup meta-server), resolver returned (1432158224)
> [sssd[be[ipaldap]]] [be_resolve_server_process] (0x1000): Trying with the
> next one!
>
> This occured when I queried a server that didn't have the query stored
> at all.
>
> The query refresh seemed to worked OK also (I tested by manually
> modifiying the timeout). Except for the uninitialized variable, I think
> the patches are working good and we just need to resolve the minor code
> concerns above.

What exactly looks wrong to you? The high error code? That's OK - it is custom sss error.

[Jakub Hrozek]

On Mon, Apr 08, 2013 at 05:05:11PM -0400, Pavel Brezina wrote:
> ----- Original Message -----
> > From: "Jakub Hrozek" 
> > To: [e-mail]
> > Sent: Monday, April 8, 2013 10:38:47 PM
> > Subject: Re: [SSSD] [PATCH] DNS site support 1st wave - generic SRV lookup plugin
> >
> > On Mon, Apr 08, 2013 at 04:51:47PM +0200, Jakub Hrozek wrote:
> > > On Mon, Apr 08, 2013 at 11:55:15AM +0200, Pavel Březina wrote:
> > > > On 04/05/2013 01:43 PM, Pavel Březina wrote:
> > > > > On 04/05/2013 12:41 PM, Pavel Březina wrote:
> > > > > > On 04/03/2013 11:48 AM, Pavel Březina wrote:
> > > > > > >On 04/02/2013 03:55 PM, Pavel Březina wrote:
> > > > > > >>On 03/27/2013 03:00 PM, Jakub Hrozek wrote:
> > > > > > >>>On Wed, Mar 27, 2013 at 11:22:49AM +0100, Pavel Březina wrote:
> > > > > > >>>>Later, we can place the plugin architecture inside a module that
> > > > > > >>>>knows about SSSD and is placed above resolver. You said you want to
> > > > > > >>>>decouple fail over to fail over -> cache -> resolver. This is nice
> > > > > > >>>>idea.
> > > > > > >>>>
> > > > > > >>>>The plugin architecture can be put inside the cache, because cache
> > > > > > >>>>is responsible for resolving expired uris and _srv_ and it does not
> > > > > > >>>>really say that is has to use resolver directly.
> > > > > > >>>
> > > > > > >>>OK, this would be the best approach. After more discussion on #sssd
> > > > > > >>>with Pavel and Simo we agreed that Pavel will carry on with this
> > > > > > >>>approach for the time being.
> > > > > > >>>
> > > > > > >>>The only two changes in these patches would be better namespacing of
> > > > > > >>>the functions used and unifying the resolver initialization with
> > > > > > >>>what
> > > > > > >>>I'm working on wrt dynamic DNS updates.
> > > > > > >>>
> > > > > > >>>In the next release, we will create the additional layer and move
> > > > > > >>>the plugins there. I moved https://fedorahosted.org/sssd/ticket/1083
> > > > > > >>>which is tracking that effort to NEEDS_TRIAGE so we can put it into
> > > > > > >>>an appropriate milestone tomorrow.
> > > > > > >>
> > > > > > >>Hi,
> > > > > > >>I'm sending new patch set.
> > > > > > >>
> > > > > > >>I created async_resolv_utils.c and put there functions for domain
> > > > > > >>detection and for resolving SRV with fallback discovery domains.
> > > > > > >>This is
> > > > > > >>similar to resolve_srv_send and resolve_get_domain_send from the
> > > > > > >>first
> > > > > > >>patchset.
> > > > > > >>
> > > > > > >>resolve_srv_send() was renamed to fo_discover_srv_send() and it wraps
> > > > > > >>resolv_discover_srv_send() so that it returns fo_server_info instead
> > > > > > >>of
> > > > > > >>ares.
> > > > > > >>
> > > > > > >>The plugin was renamed to fo_resolve_srv_dns_send(). It is a little
> > > > > > >>bit
> > > > > > >>more complicated since it performs domain detection (originally in
> > > > > > >>resolve_srv_send()) if the domain is not provided from fail over.
> > > > > > >>
> > > > > > >>Hostname is passed via plugin context. It is currently retrieved via
> > > > > > >>gethostname() so the current behaviour does not change. Later, when
> > > > > > >>the
> > > > > > >>plugin initialization will be moved to provider code, the provider
> > > > > > >>will
> > > > > > >>decide what hostname will be used (ipa_hostname for example).
> > > > > > >
> > > > > > >Rebased on top of latest be_res patches.
> > > > > >
> > > > > > When working on IPA plugin I've done some changes to this code. Since
> > > > > > it
> > > > > > is still review pending, I have squashed the changes to this patch set.
> > > > > > all SRV plugin related things are in a separate header file named
> > > > > >
> > > > > > fail_over_srv.h. This way we don't have to #include the whole fail over
> > > > > > when developing new plugin. The only thing which remained in
> > > > > > fail_over.h
> > > > > > is fo_set_srv_lookup_plugin(), because it should remain hidden for
> > > > > > providers, see #2
> > > > > > be_fo_set_srv_lookup_plugin() was added, to keep the fail over
> > > > > >
> > > > > > opaque
> > > > > > for providers.
> > > > > > the original sixth patch (set plugin for all providers) was meant to
> > > > > >
> > > > > > be reverted, once we agreed on further steps. Now it is completely
> > > > > > rewritten to final solution on which we agreed with Jakub.
> > > > > >
> > > > > > I have the IPA plugin already written (not tested yet), so I believe
> > > > > > this is the final version if there won't be any review comments.
> > > > >
> > > > > //lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > >
> > > > > OK. One more time :-)
> > > > > I have slightly enhanced debug messages.
> > > >
> > > > I forgot to check if a server is already present in the list. The fifth
> > > > patch was split in two.
> > >
> > > Patch 0001: resolv: add resolv_get_domain request to resolv utils
> > > Mostly OK, can you just explain this part of the code?
> > >
> > > 152     dns_domain = talloc_strdup(state, dns_domain);
> > > 153     if (dns_domain == NULL) {
> > > 154         return ENOMEM;
> > > 155     }
> > > 156
> > > 157     *_dns_domain = talloc_steal(mem_ctx, dns_domain);
> > > 158
> > > 159     return EOK;
> > >
> > > Isn't it better to just:
> > >     *_dns_domain = talloc_strdup(mem_ctx, dns_domain);
> > > ?
> > >
> > > Patch 0002: resolv: add resolv_discover_srv request to resolv utils
> > > Is it necessary to strdup the input variables? It might be safer in
> > > case this request outlives the request that created it but in most cases
> > > it's not really needed. The discovery_domain, on the other hand is
> > > neither checked not duped.
> > >
> > > Patch 0003: DNS sites support - SRV lookup plugin interface
> > > Code-wise looks good, but I have two questions related to the behaviour.
> > >
> > > Why can't the setter overwrite the plugin interface when it's already
> > > set? Isn't is nice for cases like LDAP where LDAP might set its generic
> > > plugin first and then IPA which uses LDAP underneath would reset them?
> > >
> > > Patch 0004: DNS sites support - SRV DNS lookup plugin
> > > Please do not use constructs like this unless EOK is expected:
> > >
> > > 66 immediately:
> > > 67     if (ret == EOK) {
> > > 68         tevent_req_done(req);
> > > 69     } else {
> > >
> > > At the very least it's confusing Coverity and showing up as "dead code".
> > >
> > > Same question about strdup-ing input parameters as for patch #2.
> > >
> > > I'm not really fond of the name "be_fo_set_standard_srv_lookup_plugin"
> > > -- standard by what means? What about using the same naming standard as
> > > in fail_over_srv.c and using be_fo_set_dns_srv_lookup_plugin ?
> > >
> > > Also since be_fo_set_standard_srv_lookup_plugin can fail and then the
> > > failover would not really be usable, it should return errno and users
> > > should check it.
> > >
> > > Patch 0005: fail over - add function to insert multiple servers to the
> > > list
> > > Ack
> > >
> > > Patch 0006:  DNS sites support - replace SRV lookup code with a plugin
> > > call
> > > Did you check if the code can be compiled with this patch? If it can,
> > > then it's fine that the lookups don't work but I would prefer the
> > > patches to compile on their own so that git-bisect is usable.
> > >
> > > Otherwise ack.
> > >
> > > Patch 0007: DNS sites support - use SRV DNS lookup plugin in all providers
> > > Mostly ack, but when I looked at how the host name is selected in LDAP
> > > initialization, it seemed to be that this could be moved down the stack
> > > either to be_fo_set_standard_srv_lookup_plugin or even lower to
> > > resolv_utils. Anyway, looks ugly in the generic LDAP code :)
> > >
> > > Patch 0008: DNS sites support - make fo_discover_srv request visible
> > > I don't get the point of this patch, why not make the functions pubilc
> > > from the start?
> > >
> > > So far I haven't tested the patches, just read the code to give some
> > > feedback. I'll reply again when I've tested it.
> >
> > My testing consisted of positive tests with the IPA (including the one
> > patch in the other thread) and LDAP backends, those went fine. Negative
> > tests worked OK, too but revealed one probably uninitialized variable:
> >
> > [sssd[be[ipaldap]]] [fo_set_port_status] (0x0100): Marking port 0 of server
> > '(no name)' as 'not working'
> > [sssd[be[ipaldap]]] [resolve_srv_done] (0x0040): Unable to resolve SRV
> > [1432158224]: SRV record not found
> >                                  ^^^^^^
> >                                  seems wrong
> > [sssd[be[ipaldap]]] [set_srv_data_status] (0x0100): Marking SRV lookup of
> > service 'LDAP' as 'not resolved'
> > [sssd[be[ipaldap]]] [be_resolve_server_process] (0x0080): Couldn't resolve
> > server (SRV lookup meta-server), resolver returned (1432158224)
> > [sssd[be[ipaldap]]] [be_resolve_server_process] (0x1000): Trying with the
> > next one!
> >
> > This occured when I queried a server that didn't have the query stored
> > at all.
> >
> > The query refresh seemed to worked OK also (I tested by manually
> > modifiying the timeout). Except for the uninitialized variable, I think
> > the patches are working good and we just need to resolve the minor code
> > concerns above.
>
> What exactly looks wrong to you? The high error code? That's OK - it is custom sss error.

Yes, but I didn't realize the new error code base was set so high
(0x555D0000) Sorry, carry on :)

[Jakub Hrozek]

On Mon, Apr 08, 2013 at 04:51:47PM +0200, Jakub Hrozek wrote:
> There is quite some new functionality in this patchset that would be
> great to have covered by unit-tests. I realize we need to finish the
> code first and foremost, so maybe we could file tickets (trivial or
> minor) and fix them in 1.10 post-beta? But I would like to start
> building the coverage, if we don't even file the ticket, we'll forget
> completely.

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

[Pavel Březina]

On 03/25/2013 05:18 PM, Jakub Hrozek wrote:
> > +static void resolve_srv_step1_done(struct tevent_req *subreq);
>
> Ugh, step1 is not the greatest name :)

Actually, this is suggested by tevent documentation :-)

[Pavel Březina]

On 03/25/2013 05:18 PM, Jakub Hrozek wrote:
> On Fri, Mar 22, 2013 at 02:47:31PM +0100, Pavel Březina wrote:
> > Hi, this patchset takes the current SRV lookup code and converts it
> > into a plugin. Next waves will add custom plugin for IPA and AD.
> >
> > Patch 1: Introduce new plugin interface.
> >
> > Patch 2: First SRV lookup plugin.
> >
> > Patch 3: Replaces current code with a plugin code. I originally
> > wanted to remove resolve_srv_* functions completely and call the
> > plugin in fo_resolve_service_send() directly but that proved to be
> > harder and more error sensitive than I expected.
> >
> > This got me thinking, we should create a refactoring ticket for
> > the failover code. It has become quite complex and hard to read
> > and understand because it lacks a lot of comments. Also many things
> > there seems very hackish to me.
>
> Feel free to create that ticket, I agree that the current failover
> code has grown too much..but keep in mind that many of the "hacks"
> are in fact special cases for all kinds of weird errors.
>
> My idea was to decouple the caching logic in the failover to a
> separate layer. So resolver would always talk to DNS or whatever back
> end there was, then there would be a resolver cache and finally fail
> over that would only manage states of servers and ports.
>
> Is there anything else that bothers you with the current failover
> architecture?
> > Patch 4: Set SRV lookup plugin for all providers. This is done as a
> > separate patch because it will be reverted once each provider has
> > its own plugin.
> >
> > I also would like to discuss where we can set the plugin in the
> > future. At the moment we have one failover context for the whole
> > backend. But the backend can be configured to run several different
> > providers at the same time. The plugin itself is made per provider
> > you have different plugin for IPA and different for LDAP.
> >
> > I think we should have failover context per provider. My idea is
> > to create a new module constructor e.g. sssm_ipa_init(). This will
> > be called only once before all other sssm_ipa_*_init() functions
> > and it will initialize IPA-wise failover context.
>
> In theory there may be a case where we need this functionality, yes.
> I can't think of any use-case right now, but this may be a better
> architecture going forward, especially as we are adding more
> providers like sudo or autofs that are only implemented with some
> back ends.
> > We can also use it to initialize sdap_id_ctx instead of calling
> > sssm_ipa_id_init from other places.

Given it more thoughts I don't think we should go this way. I like the
sssm_ipa_init() concept though and we should probably implement it in
1.11+.

But there is an advantage having only one plugin active. For example:

id_provider = ipa
sudo_provider = ldap
ldap_uri = _srv_

Even though sudo_provider is ldap we still contact ipa server so it is
logical to still use plugin with ipa sites support.

We can consider id_provider as authoritative provider and set the plugin
accordingly. Any thoughts?

[Jakub Hrozek]

On Wed, Apr 03, 2013 at 01:26:12PM +0200, Pavel Březina wrote:
> > > We can also use it to initialize sdap_id_ctx instead of calling
> > > sssm_ipa_id_init from other places.
>
> Given it more thoughts I don't think we should go this way. I like the
> sssm_ipa_init() concept though and we should probably implement it in
> 1.11+.
>
> But there is an advantage having only one plugin active. For example:
>
> id_provider = ipa
> sudo_provider = ldap
> ldap_uri = _srv_
>
> Even though sudo_provider is ldap we still contact ipa server so it is
> logical to still use plugin with ipa sites support.
>
> We can consider id_provider as authoritative provider and set the plugin
> accordingly. Any thoughts?

This case is only valid for complex providers like IPA or AD, right?
Then for 1.10 this seems like a good and simple idea.

Long-term I'm actually thinking of creating a "hosts provider" or
"resolv provider" that would inherit from id_provider but could be used
to configure the host resolution with a nice granularity. But it's too
late for 1.10 I think.