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.
Martin
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.
Martin, why there are so many static global variables in this code ? (static struct { ... } global ....)
Why aren't you using a context based approach like all the rest of the code in sssd ?
Simo.
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.
Martin, why there are so many static global variables in this code ? (static struct { ... } global ....)
Why aren't you using a context based approach like all the rest of the code in sssd ?
Simo.
My reason was that I thought it might not be easy to share one context. If I would use a context base approach then we would have to make sure that every provider uses the same one. If you think this is not a problem I'll gladly change the patch. Since a pointer to a context could be stored in struct service for example, you would only need it in few calls anyway.
Martin
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.
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.
Martin
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.
On Thu, 2009-11-05 at 15:53 -0500, Simo Sorce wrote:
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:
[snip]
Simo.
OK, should be good now. I also optimized a bit functions for getting server/port status so they don't always call gettimeofday() if they don't have to and I also made the code a bit cleaner. New patches rebased on the current master are attached.
Martin
On Fri, 2009-11-06 at 17:50 +0100, Martin Nagy wrote:
On Thu, 2009-11-05 at 15:53 -0500, Simo Sorce wrote:
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:
[snip]
Simo.
OK, should be good now. I also optimized a bit functions for getting server/port status so they don't always call gettimeofday() if they don't have to and I also made the code a bit cleaner. New patches rebased on the current master are attached.
Ok, we have betean this horse enough, Ack and pushed.
Simo.
sssd-devel@lists.fedorahosted.org
-
Martin Nagy -
Simo Sorce