[Openais] [corosync] - cpg_model_initialize

Steven Dake sdake at redhat.com
Wed Mar 24 14:27:43 PDT 2010


looks pretty good

few comments inline

There should likely be the addition of the model for dct's case which
could be modelv2 or just merged into modelv1.

Also need manpage

regards
-steve


On Mon, 2010-03-22 at 18:01 +0100, Jan Friesse wrote:
> Attached is patch implementing cpg_model_initialize functionality. It's
> still little proof of concept.
> 
> Regards,
>   Honza
> plain text document attachment (2010-03-22-cpg_initialize_model.patch)
> commit 9ab47f9ddc4f6824f568e194532fcc3acbd532ea
> Author: Jan Friesse <jfriesse at redhat.com>
> Date:   Mon Mar 22 17:44:16 2010 +0100
> 
>     CPG model_initialize
>     
>     Patch adds new function to initialize cpg, cpg_model_initialize. Model
>     is set of callbacks. With this function, future addions of models should
>     be possible without changing the ABI.
> 
> diff --git a/trunk/include/corosync/cpg.h b/trunk/include/corosync/cpg.h
> index b5609df..25c3680 100644
> --- a/trunk/include/corosync/cpg.h
> +++ b/trunk/include/corosync/cpg.h
> @@ -78,6 +78,10 @@ typedef enum {
>  	CPG_ITERATION_ALL = 3,
>  } cpg_iteration_type_t;
>  
> +typedef enum {
> +	CPG_MODEL_V1 = 1,
> +} cpg_model_t;
> +
>  struct cpg_address {
>  	uint32_t nodeid;
>  	uint32_t pid;
> @@ -122,6 +126,17 @@ typedef struct {
>  	cpg_confchg_fn_t cpg_confchg_fn;
>  } cpg_callbacks_t;
>  
> +typedef struct {
> +	cpg_model_t model;
> +} cpg_model_callbacks_t;
> +
> +typedef struct {
> +	cpg_model_t model_v1;

It may make more sense to pass the model in explicitly into the function
call.  Looking at this struct as an outside user it isn't immediately
clear to me what I should set model_v1 to or if it needs to be set at
all..


> +	cpg_deliver_fn_t cpg_deliver_fn;
> +	cpg_confchg_fn_t cpg_confchg_fn;
> +} cpg_model_v1_callbacks_t;
> +
> +
>  /** @} */
>  
>  /*
> @@ -132,6 +147,14 @@ cs_error_t cpg_initialize (
>  	cpg_callbacks_t *callbacks);
>  
>  /*
> + * Create a new cpg connection, initialize with model
> + */
> +cs_error_t cpg_model_initialize (
> +	cpg_handle_t *handle,
> +	cpg_model_callbacks_t *callbacks,
> +	void *context);
> +
> +/*
>   * Close the cpg handle
>   */
>  cs_error_t cpg_finalize (
> diff --git a/trunk/lib/cpg.c b/trunk/lib/cpg.c
> index 79836e4..4d0854e 100644
> --- a/trunk/lib/cpg.c
> +++ b/trunk/lib/cpg.c
> @@ -62,8 +62,11 @@
>  struct cpg_inst {
>  	hdb_handle_t handle;
>  	int finalize;
> -	cpg_callbacks_t callbacks;
>  	void *context;
> +	union {
> +		cpg_model_callbacks_t model_callbacks;
> +		cpg_model_v1_callbacks_t model_v1_callbacks;
> +	};
>  	struct list_head iteration_list_head;
>  };
>  
> @@ -118,9 +121,33 @@ cs_error_t cpg_initialize (
>  	cpg_handle_t *handle,
>  	cpg_callbacks_t *callbacks)
>  {
> +	cpg_model_v1_callbacks_t model_v1_callbacks;
> +
> +	memset (&model_v1_callbacks, 0, sizeof (cpg_model_v1_callbacks_t));
> +
> +	model_v1_callbacks.model_v1 = CPG_MODEL_V1;
> +
> +	if (callbacks) {
> +		model_v1_callbacks.cpg_deliver_fn = callbacks->cpg_deliver_fn;
> +		model_v1_callbacks.cpg_confchg_fn = callbacks->cpg_confchg_fn;
> +	}
> +

If callbacks is set to null, which is valid for cpg_initialize,
cpg_model_initialize will instead pass a callback list of
model_v1_callbacks.  I believe you want to pass NULL if callbacks is
null the the 2nd parameter.

> +	return (cpg_model_initialize (handle, (cpg_model_callbacks_t *)&model_v1_callbacks, NULL));
> +}
> +
> +cs_error_t cpg_model_initialize (
> +	cpg_handle_t *handle,
> +	cpg_model_callbacks_t *callbacks,
> +	void *context)
> +{
>  	cs_error_t error;
>  	struct cpg_inst *cpg_inst;
>  
> +	if (callbacks != NULL && callbacks->model != CPG_MODEL_V1) {
> +		error = CPG_ERR_INVALID_PARAM;
> +		goto error_no_destroy;
> +	}

callbacks = null is valid, it just means no callbacks will be executed.
Make error checks like this separate lines so ppl that debug into the
libs can immediately see what condition triggered the branch.


> +
>  	error = hdb_error_to_cs (hdb_handle_create (&cpg_handle_t_db, sizeof (struct cpg_inst), handle));
>  	if (error != CS_OK) {
>  		goto error_no_destroy;
> @@ -143,7 +170,20 @@ cs_error_t cpg_initialize (
>  	}
>  
>  	if (callbacks) {
> -		memcpy (&cpg_inst->callbacks, callbacks, sizeof (cpg_callbacks_t));
> +		switch (callbacks->model) {
> +		case CPG_MODEL_V1:
> +			memcpy (&cpg_inst->model_v1_callbacks, callbacks, sizeof (cpg_model_v1_callbacks_t));
> +			break;
> +		default:
> +			error = CS_ERR_LIBRARY;
> +			goto error_destroy;
> +			break;
> +		}
> +	} else {
> +		/*
> +		 * NULL callbacks is effectively v1 without callbacks
> +		 */
> +		cpg_inst->model_v1_callbacks.model_v1 = CPG_MODEL_V1;
>  	}
>  
>  	list_init(&cpg_inst->iteration_list_head);
> @@ -283,7 +323,7 @@ cs_error_t cpg_dispatch (
>  	struct cpg_inst *cpg_inst;
>  	struct res_lib_cpg_confchg_callback *res_cpg_confchg_callback;
>  	struct res_lib_cpg_deliver_callback *res_cpg_deliver_callback;
> -	cpg_callbacks_t callbacks;
> +	struct cpg_inst cpg_callbacks;
>  	coroipc_response_header_t *dispatch_data;
>  	struct cpg_address member_list[CPG_MEMBERS_MAX];
>  	struct cpg_address left_list[CPG_MEMBERS_MAX];
> @@ -332,66 +372,75 @@ cs_error_t cpg_dispatch (
>  		 * A risk of this dispatch method is that the callback routines may
>  		 * operate at the same time that cpgFinalize has been called.
>  		 */
> -		memcpy (&callbacks, &cpg_inst->callbacks, sizeof (cpg_callbacks_t));
> +		memcpy (&cpg_callbacks, cpg_inst, sizeof (struct cpg_inst));
>  		/*
>  		 * Dispatch incoming message

This part could use restructure to be a little more clear. Something
like:
		switch cpg_callbacks.model_callbacks.model) {
		case CPG_MODEL_V1:
			then the switch dispatch

That way it is easy to determine which callbacks belong to which model.


>  		 */
>  		switch (dispatch_data->id) {
>  		case MESSAGE_RES_CPG_DELIVER_CALLBACK:
> -			if (callbacks.cpg_deliver_fn == NULL) {
> -				continue;
> +			switch (cpg_callbacks.model_callbacks.model) {
> +			case CPG_MODEL_V1:
> +				if (cpg_callbacks.model_v1_callbacks.cpg_deliver_fn == NULL) {
> +					continue;
> +				}
> +
> +				res_cpg_deliver_callback = (struct res_lib_cpg_deliver_callback *)dispatch_data;
> +
> +				marshall_from_mar_cpg_name_t (
> +					&group_name,
> +					&res_cpg_deliver_callback->group_name);
> +
> +				cpg_callbacks.model_v1_callbacks.cpg_deliver_fn (handle,
> +					&group_name,
> +					res_cpg_deliver_callback->nodeid,
> +					res_cpg_deliver_callback->pid,
> +					&res_cpg_deliver_callback->message,
> +					res_cpg_deliver_callback->msglen);
> +				break;
>  			}
> -
> -			res_cpg_deliver_callback = (struct res_lib_cpg_deliver_callback *)dispatch_data;
> -
> -			marshall_from_mar_cpg_name_t (
> -				&group_name,
> -				&res_cpg_deliver_callback->group_name);
> -
> -			callbacks.cpg_deliver_fn (handle,
> -				&group_name,
> -				res_cpg_deliver_callback->nodeid,
> -				res_cpg_deliver_callback->pid,
> -				&res_cpg_deliver_callback->message,
> -				res_cpg_deliver_callback->msglen);
>  			break;
>  
>  		case MESSAGE_RES_CPG_CONFCHG_CALLBACK:
> -			if (callbacks.cpg_confchg_fn == NULL) {
> -				continue;
> -			}
> -
> -			res_cpg_confchg_callback = (struct res_lib_cpg_confchg_callback *)dispatch_data;
> -
> -			for (i = 0; i < res_cpg_confchg_callback->member_list_entries; i++) {
> -				marshall_from_mar_cpg_address_t (&member_list[i],
> -					&res_cpg_confchg_callback->member_list[i]);
> -			}
> -			left_list_start = res_cpg_confchg_callback->member_list +
> -				res_cpg_confchg_callback->member_list_entries;
> -			for (i = 0; i < res_cpg_confchg_callback->left_list_entries; i++) {
> -				marshall_from_mar_cpg_address_t (&left_list[i],
> -					&left_list_start[i]);
> -			}
> -			joined_list_start = res_cpg_confchg_callback->member_list +
> -				res_cpg_confchg_callback->member_list_entries +
> -				res_cpg_confchg_callback->left_list_entries;
> -			for (i = 0; i < res_cpg_confchg_callback->joined_list_entries; i++) {
> -				marshall_from_mar_cpg_address_t (&joined_list[i],
> -					&joined_list_start[i]);
> +			switch (cpg_callbacks.model_callbacks.model) {
> +			case CPG_MODEL_V1:
> +				if (cpg_callbacks.model_v1_callbacks.cpg_confchg_fn == NULL) {
> +					continue;
> +				}
> +
> +				res_cpg_confchg_callback = (struct res_lib_cpg_confchg_callback *)dispatch_data;
> +
> +				for (i = 0; i < res_cpg_confchg_callback->member_list_entries; i++) {
> +					marshall_from_mar_cpg_address_t (&member_list[i],
> +						&res_cpg_confchg_callback->member_list[i]);
> +				}
> +				left_list_start = res_cpg_confchg_callback->member_list +
> +					res_cpg_confchg_callback->member_list_entries;
> +				for (i = 0; i < res_cpg_confchg_callback->left_list_entries; i++) {
> +					marshall_from_mar_cpg_address_t (&left_list[i],
> +						&left_list_start[i]);
> +				}
> +				joined_list_start = res_cpg_confchg_callback->member_list +
> +					res_cpg_confchg_callback->member_list_entries +
> +					res_cpg_confchg_callback->left_list_entries;
> +				for (i = 0; i < res_cpg_confchg_callback->joined_list_entries; i++) {
> +					marshall_from_mar_cpg_address_t (&joined_list[i],
> +						&joined_list_start[i]);
> +				}
> +				marshall_from_mar_cpg_name_t (
> +					&group_name,
> +					&res_cpg_confchg_callback->group_name);
> +
> +				cpg_callbacks.model_v1_callbacks.cpg_confchg_fn (handle,
> +					&group_name,
> +					member_list,
> +					res_cpg_confchg_callback->member_list_entries,
> +					left_list,
> +					res_cpg_confchg_callback->left_list_entries,
> +					joined_list,
> +					res_cpg_confchg_callback->joined_list_entries);
> +
> +				break;
>  			}
> -			marshall_from_mar_cpg_name_t (
> -				&group_name,
> -				&res_cpg_confchg_callback->group_name);
> -
> -			callbacks.cpg_confchg_fn (handle,
> -				&group_name,
> -				member_list,
> -				res_cpg_confchg_callback->member_list_entries,
> -				left_list,
> -				res_cpg_confchg_callback->left_list_entries,
> -				joined_list,
> -				res_cpg_confchg_callback->joined_list_entries);
>  			break;
>  
>  		default:
> _______________________________________________
> Openais mailing list
> Openais at lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/openais



More information about the Openais mailing list