[SSSD] [PATCH] Lookup domains at startup

Jakub Hrozek jhrozek at redhat.com
Tue Jun 4 12:18:35 UTC 2013


On Tue, Jun 04, 2013 at 12:05:12PM +0200, Sumit Bose wrote:
> On Fri, May 31, 2013 at 04:15:18PM +0200, Jakub Hrozek wrote:
> > On Fri, May 31, 2013 at 01:37:11PM +0200, Sumit Bose wrote:
> > > Hi,
> > > 
> > > recently the patch "Allow flat name in the FQname format" was commit to
> > > master. The flat domain name is determined at runtime but currently only when
> > > the responders receive a request with an unknown domain name.
> > > 
> > > If now the flat domain name is used in the FQname and the nss responder
> > > receives e.g. a 'getent passwd DOM\username' request with the flat
> > > domain name after startup everything is fine. Because after startup the
> > > domain part of the given fully qualified user name is not know and a
> > > request will be send to the backends to look it up. If the request is
> > > done the flat domain name is know and can be used in the returned
> > > FQname.
> > > 
> > > if on the other hand the nss responder receives a 'getent passwd
> > > username at domain.name' with the domain name from sssd.conf the domain
> > > part of the user name is known and there is no reason to send a
> > > get_domains request to the backend. Hence the flat domain name is not
> > > known when the FQname for the response is constructed and will be
> > > replaced by the full name.
> > > 
> > > To avoid this the following patch will always run a get_domains request
> > > at startup to get the needed domain data.
> > > 
> > > Fixes https://fedorahosted.org/sssd/ticket/1951.
> > > 
> > > bye,
> > > Sumit
> > 
> > Works fine, after startup there is a subdomain created and the flat name
> > discovered.
> > 
> > But I wonder if it was worth it to add some kind of check in
> > be_get_subdomains() to avoid issuing another request if the previous
> > came within some interval and if it did just return success. Currently
> > this patch triggers an LDAP search per responder.
> > 
> > The downside I see is that there is already a similar check in the
> > responders themselves, so this would be a bit of duplication.
> 
> Thank you for the review. I've added two patches which adds a generic
> request queue to the data provider code and let the get_subdomains
> request use it. With this you should only see two requests going to the
> server, one triggered by the starting responders and the other is the
> online callback. I think it is not worth the time to try to optimize
> this out as well, because there are different requirements for the
> responders and the online callback.
> 
> bye,
> Sumit

Thank you, this is much better, with the additional check in the AD
subdomain code to shortcut back, I no longer see a search per responder.

[PATCH 1/3] Lookup domains at startup
Ack

[PATCH 2/3] Add be request queue
Just one small change below, I can change the part before pushing if you
agree:

> From d3482d3a25f2e65749a3d988938f7e7183e45880 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Fri, 31 May 2013 22:00:17 +0200
> Subject: [PATCH 2/3] Add be request queue
> 
> For some backend targets it might be not desirable to run requests in
> parallel but to serialize them. To avoid that each provider has to
> implement a queue for this target this patch implements a generic queue
> which collects incoming requests before they are send to the target.
> ---
>  src/providers/data_provider_be.c |  119 ++++++++++++++++++++++++++++++++++++++
>  src/providers/dp_backend.h       |   11 ++++
>  2 files changed, 130 insertions(+), 0 deletions(-)
> 
> diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
> index cd67156..49a7a89 100644
> --- a/src/providers/data_provider_be.c
> +++ b/src/providers/data_provider_be.c
> @@ -301,6 +301,125 @@ static errno_t be_file_request(TALLOC_CTX *mem_ctx,
>      return EOK;
>  }
>  
> +static errno_t be_queue_request(TALLOC_CTX *queue_mem_ctx,
> +                                struct bet_queue_item **req_queue,
> +                                TALLOC_CTX *req_mem_ctx,
> +                                struct be_req *be_req,
> +                                be_req_fn_t fn)
> +{
> +    struct bet_queue_item *item;
> +    int ret;
> +
> +    if (*req_queue == NULL) {
> +        DEBUG(SSSDBG_TRACE_ALL, ("Queue is empty, " \
> +                                 "running request immediately.\n"));
> +        ret = be_file_request(req_mem_ctx, be_req, fn);
> +        if (ret != EOK) {
> +            DEBUG(SSSDBG_OP_FAILURE, ("be_file_request failed.\n"));
> +            return ret;
> +        }
> +    }
> +
> +    item = talloc_zero(queue_mem_ctx, struct bet_queue_item);
> +    if (item == NULL) {
> +        DEBUG(SSSDBG_OP_FAILURE, ("talloc_zero failed, cannot add item to " \
> +                                  "request queue.\n"));
> +    } else {
> +        DEBUG(SSSDBG_TRACE_ALL, ("Adding request to queue.\n"));
> +        item->mem_ctx = req_mem_ctx;
> +        item->be_req = be_req;
> +        item->fn = fn;
> +
> +        DLIST_ADD(*req_queue, item);

DLIST_ADD adds to the front of the list and when talking about "queues",
typically the user adds to the tail of the list to keep the FIFO
behaviour. I think DLIST_ADD_END() would read better here.

> +    }
> +
> +    return EOK;
> +}
> +

The rest of the patch looks fine to me.

[PATCH 3/3] Use queue for get_subdomains
Ack



More information about the sssd-devel mailing list