If this patch fixes the problem then I don't see any strong motivation
to change around ipc.
Regards
-steve
On 01/17/2012 06:49 AM, Fabio M. Di Nitto wrote:
> Steven,
>
> the problem here, introduced by loading always all services, is that
> associated libraries will always allow connections. This is somewhat a
> behavior regression compared to not loading the services all the time.
>
> Now, most services can deal with it just fine, but in the votequorum
> example, we refuse to handle a votequorum_handle when the service
> "was/is" not loaded. Effectively reporting the libvotequorum users that
> the service was not configured/loaded (it was previous linked in by
> vfs_quorum).
>
> This change, basically init/link votequorum only when necessary.
>
> Another possibility, but it´s more intrusive in other areas, is to
> actually make lib_init_fn useful.
>
> In the current IPC implementation, lib_init_fn is called in
> _connection_created (exec/ipc_glue.c). Return code from lib_init_fn is
> not even taken into account and _connection_created returns void.
>
> In _theory_ we could move lib_init_fn into _connection_accept but I am
> not sure if it´s possible to lib_init at that stage. Then check errors
> from lib_init_fn and eventually fail the _accept, propagating the issue
> down IPC -> libfoo_initialize.
>
> For now I am pushing this one as fix, so people can keep working, but
> please think a second if we should go the other route. testing of each
> single library is still required at this point to make sure that they
> don´t segfault or whatever. In votequorum I was clearly experiencing
> crashes since libvotequorum was attempting to access data that were
> never created or assigned.
>
> Fabio
>
> On 1/17/2012 2:38 PM, Fabio M. Di Nitto wrote:
>> From: "Fabio M. Di Nitto" <fdinitto@xxxxxxxxxx>
>>
>> the problem is that votequorum was listed as part of default
>> services.
>>
>> At service_link_and_init, votequorum library and exec were being
>> made available, even when votequorum was not in used at all, creating
>> all kind of problems.
>>
>> By changing the service_link_and_init to be clever, we restore the
>> original and wanted behavior to link the service only when required.
>>
>> This also fixed N*votequorum API calls segfaults, init segfaults
>> and a few dozen other small issues...
>>
>> Signed-off-by: Fabio M. Di Nitto <fdinitto@xxxxxxxxxx>
>> ---
>> exec/service.c | 11 -----------
>> exec/service.h | 6 +++++-
>> exec/votequorum.c | 25 +++++++++++++------------
>> exec/vsf_ykd.c | 8 --------
>> 4 files changed, 18 insertions(+), 32 deletions(-)
>>
>> diff --git a/exec/service.c b/exec/service.c
>> index 1432146..4cd9d8e 100644
>> --- a/exec/service.c
>> +++ b/exec/service.c
>> @@ -58,12 +58,6 @@
>>
>> LOGSYS_DECLARE_SUBSYS ("SERV");
>>
>> -struct default_service {
>> - const char *name;
>> - int ver;
>> - struct corosync_service_engine *(*loader)(void);
>> -};
>> -
>> static struct default_service default_services[] = {
>> {
>> .name = "corosync_evs",
>> @@ -100,11 +94,6 @@ static struct default_service default_services[] = {
>> },
>> #endif
>> {
>> - .name = "corosync_votequorum",
>> - .ver = 0,
>> - .loader = votequorum_get_service_engine_ver0
>> - },
>> - {
>> .name = "corosync_quorum",
>> .ver = 0,
>> .loader = vsf_quorum_get_service_engine_ver0
>> diff --git a/exec/service.h b/exec/service.h
>> index 5309174..67d4b94 100644
>> --- a/exec/service.h
>> +++ b/exec/service.h
>> @@ -39,7 +39,11 @@
>>
>> struct corosync_api_v1;
>>
>> -struct default_service;
>> +struct default_service {
>> + const char *name;
>> + int ver;
>> + struct corosync_service_engine *(*loader)(void);
>> +};
>>
>> /**
>> * Link and initialize a service
>> diff --git a/exec/votequorum.c b/exec/votequorum.c
>> index 0f4f05a..ef0e3a7 100644
>> --- a/exec/votequorum.c
>> +++ b/exec/votequorum.c
>> @@ -58,7 +58,6 @@ LOGSYS_DECLARE_SUBSYS ("VOTEQ");
>> */
>>
>> static struct corosync_api_v1 *corosync_api;
>> -static int votequorum_configured = 0;
>>
>> /*
>> * votequorum global config vars
>> @@ -222,6 +221,8 @@ static quorum_set_quorate_fn_t quorum_callback;
>> * votequorum_exec handler and definitions
>> */
>>
>> +static int votequorum_exec_init_fn (struct corosync_api_v1 *api);
>> +
>> static void message_handler_req_exec_votequorum_nodeinfo (
>> const void *message,
>> unsigned int nodeid);
>> @@ -328,6 +329,7 @@ static struct corosync_service_engine votequorum_service_engine = {
>> .lib_exit_fn = quorum_lib_exit_fn,
>> .lib_engine = quorum_lib_service,
>> .lib_engine_count = sizeof (quorum_lib_service) / sizeof (struct corosync_lib_handler),
>> + .exec_init_fn = votequorum_exec_init_fn,
>> .exec_engine = votequorum_exec_engine,
>> .exec_engine_count = sizeof (votequorum_exec_engine) / sizeof (struct corosync_exec_handler),
>> .confchg_fn = votequorum_confchg_fn,
>> @@ -339,6 +341,14 @@ struct corosync_service_engine *votequorum_get_service_engine_ver0 (void)
>> return (&votequorum_service_engine);
>> }
>>
>> +static struct default_service votequorum_service[] = {
>> + {
>> + .name = "corosync_votequorum",
>> + .ver = 0,
>> + .loader = votequorum_get_service_engine_ver0
>> + },
>> +};
>> +
>> /*
>> * common/utility macros/functions
>> */
>> @@ -1131,11 +1141,6 @@ static void votequorum_confchg_fn (
>>
>> ENTER();
>>
>> - if (votequorum_configured == 0) {
>> - LEAVE();
>> - return;
>> - }
>> -
>> if (member_list_entries > 1) {
>> first_trans = 0;
>> }
>> @@ -1205,12 +1210,8 @@ cs_error_t votequorum_init(struct corosync_api_v1 *api,
>>
>> votequorum_readconfig_static();
>>
>> - if (votequorum_exec_init_fn(api) != 0) {
>> - LEAVE();
>> - return CS_ERR_NO_MEMORY;
>> - }
>> -
>> - votequorum_configured = 1;
>> + corosync_service_link_and_init(corosync_api,
>> + &votequorum_service[0]);
>>
>> LEAVE();
>>
>> diff --git a/exec/vsf_ykd.c b/exec/vsf_ykd.c
>> index 282ba7f..e021ad1 100644
>> --- a/exec/vsf_ykd.c
>> +++ b/exec/vsf_ykd.c
>> @@ -146,8 +146,6 @@ hdb_handle_t schedwrk_state_send_callback_handle;
>>
>> static struct corosync_api_v1 *api;
>>
>> -static int ykd_configured = 0;
>> -
>> static void (*ykd_primary_callback_fn) (
>> const unsigned int *view_list,
>> size_t view_list_entries,
>> @@ -465,10 +463,6 @@ static void ykd_confchg_fn (
>> {
>> int i;
>>
>> - if (ykd_configured == 0) {
>> - return;
>> - }
>> -
>> if (configuration_type != TOTEM_CONFIGURATION_REGULAR) {
>> return;
>> }
>> @@ -525,8 +519,6 @@ cs_error_t ykd_init (
>> return CS_ERR_INVALID_PARAM;
>> }
>>
>> - ykd_configured = 1;
>> -
>> api->tpg_init (
>> &ykd_group_handle,
>> ykd_deliver_fn,
>
> _______________________________________________
> discuss mailing list
> discuss@xxxxxxxxxxxx
> http://lists.corosync.org/mailman/listinfo/discuss
_______________________________________________
discuss mailing list
discuss@xxxxxxxxxxxx
http://lists.corosync.org/mailman/listinfo/discuss
