[SSSD] [PATCHES] Fail over helper functions and reference counting wrappers

Simo Sorce ssorce at redhat.com
Thu Nov 5 20:53:31 UTC 2009


On Wed, 2009-11-04 at 18:32 +0100, Martin Nagy wrote:
> On Mon, 2009-11-02 at 16:02 -0500, Simo Sorce wrote:
> > On Fri, 2009-10-30 at 14:07 +0100, Martin Nagy wrote:
> > > Hi,
> > > attached are patches needed for the fail over functionality. The
> > > service discovery is not there yet, I want to hold of with that until
> > > I
> > > have at least a basic SRV-based one so I can test it properly. It's
> > > possible that we will discover something missing when we'll be
> > > integrating it into providers. Together with Steven we at least
> > > figured
> > > out that for ldapi:// for example we need an "extra" treatment. So I
> > > made the name resolution optional and you can provide a server with
> > > user data. The commit messages and header files should explain it
> > > better.
> > 
> > Just a formal NACK.
> > We discussed changes on IRC.
> > 
> > Simo.
> 
> New patches attached.
> 
> Changes:
> * use a fail over context instead of a global variable
> * rename fo_get_server_* to fo_resolve_service_*
> * rename fo_set_service_status() and enum service_status to use
> "port_status"
> * don't use a default port for services
> * always use only one request for resolving and add even the first
> request as a subrequest into the queue
> * put user_data in fo_add_server()
> * use NULL as a server name to indicate we don't want to do resolving
> instead of having a boolean for it
> * in fo_resolve_service_send() only return NULL if the
> tevent_req_create() fails
> * add a destructor for lookup hooks, use a different memory context
> * detect if existing server name, port and user_data combination already
> exists and return EEXIST if they do
> 
> Hope I didn't miss anything.

Sorry still a NACK, this is the discussion on IRC:

<simo> mnagy, if there isn't any particular reason it should be
allocated on the fo_ctx I think
<mnagy> simo: I do? uhm, I wanted to change that actually
<simo> mnagy, yes it does, I think you can remove memctx fro
fo_new_service()
<mnagy> yes, I agree


<simo> mnagy, for consistency with the service interface, should we
return the fo_server structure in fo_new_server() ?
<mnagy> simo: there's no need
<simo> ok


<simo> mnagy, I have to nack fo_resolve_service_send() again I think
<simo> mnagy, doesn't work the way you did it
<mnagy> simo: why?
<simo> mnagy, look at fo_resolve_service_done()
<simo> mnagy, req = tevent_req_callback_data(subreq, struct tevent_req);
<simo> what is that req ?
<simo> (actually it is probably worst then before in this form :-/)
<mnagy> simo: ouch. I left that there from the old patch :/
<mnagy> shouldn't be there
<simo> mnagy,  in fo_resolve_service_send() you should probably change
tevent_req_set_callback(subreq, fo_resolve_service_done, req); with
tevent_req_set_callback(subreq, fo_resolve_service_done, common);
<simo> err server->common that is
<mnagy> yep
<simo> mnagy, set_lookup_hook() is also wrong
<simo> you need to allocate using req as the memory context not
server->common
<simo> mnagy, also remove *ev from resolve_service_request structure,
you don;t need nor wnat it there
<simo> mnagy, there is also no need to call DLIST_REMOVE in
fo_resolve_service() if you correctly allocate request over the req in
set_lookup_hook
<simo> (although you have to keep the next_request thing, as the request
will be most probably freed when you call tevent_req_done(req)
<simo> mnagy, actually it looks a bit dangerous to parse the list that
way there
<simo> mnagy, you should probably loop on the head of the list only
<simo> this way:
<simo> while (request = common->request_list)
{ DLIST_REMOVE(common->request_list, request);  ... }
<simo> mnagy, this way if there were nested requests coming from the
caller and multiple requests are freed while in tevent_req_done() you
don't try to dereference freed memory (next_request may be freed)
<simo> mnagy, acck this ? ^^
<mnagy> simo: I guess
<mnagy> simo: could you please sum up all the points you made and send
them to the list?


Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list