On 1/17/2012 3:57 PM, Steven Dake wrote:
> If this patch fixes the problem then I don't see any strong motivation
> to change around ipc.
Well yes the patch fixes the issue and since you are ok with it, there
is no strong motivation to change IPC, *but* the fact that lib_init_fn
return code is not checked and services cannot decide who connects/when,
I think it´s still a bug that should be addressed, just not right away,
since the major motivator has somehow disappeared.
Fabio
>
> 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
