[Ocfs2-tools-devel] [PATCH 05/25] libo2cb: Adds support for global heartbeat

[Joel Becker]

On Wed, Jun 23, 2010 at 11:44:15AM -0700, Sunil Mushran wrote:
> +errcode_t o2cb_list_hbregions(char *cluster_name, char ***regions);
> +void o2cb_free_hbregions_list(char **regions);

	There's no need to squish hb and regions.  We don't do this
elsewhere.  o2cb_list_hb_regions() isn't that much longer.  don't worry,
I'm not going to ask for o2cb_list_heartbeat_regions(). ;-)

> +errcode_t o2cb_start_heartbeat(struct o2cb_cluster_desc *cluster,
> +			       struct o2cb_region_desc *region)
> +{
> +	errcode_t ret, up_ret;
> +	int semid, global = 0;
> +
> +	ret = o2cb_mutex_down_lookup(region->r_name, &semid);
> +	if (ret)
> +		return ret;
> +
> +	ret = o2cb_global_heartbeat_mode(cluster->c_cluster, &global);
> +	if (ret)
> +		goto up;
> +
> +	ret = o2cb_create_heartbeat_region(cluster->c_cluster,
> +					   region->r_name,
> +					   region->r_device_name,
> +					   region->r_block_bytes,
> +					   region->r_start_block,
> +					   region->r_blocks);
> +	if (ret && ret != O2CB_ET_REGION_EXISTS)
> +		goto up;
> +
> +	if (global && ret == O2CB_ET_REGION_EXISTS)
> +		goto up;

	Shouldn't this be returning 0?  If the region already exists in
a global config, isn't that OK?

> +errcode_t o2cb_global_heartbeat_mode(char *cluster_name, int *global)
> +{
> +	char attr_path[PATH_MAX];
> +	char _fake_cluster_name[NAME_MAX];
> +	char attr_value[16], *p = attr_value;
> +	errcode_t ret;
> +
> +	if (!cluster_name) {
> +		ret = _fake_default_cluster(_fake_cluster_name);
> +		if (ret)
> +			return ret;
> +		cluster_name = _fake_cluster_name;
> +	}
> +
> +	ret = snprintf(attr_path, PATH_MAX - 1, O2CB_FORMAT_HEARTBEAT_MODE,
> +		       configfs_path, cluster_name);
> +	if ((ret <= 0) || (ret == (PATH_MAX - 1)))
> +		return O2CB_ET_INTERNAL_FAILURE;
> +
> +	*global = 0;
> +
> +	ret = o2cb_get_attribute(attr_path, attr_value, sizeof(attr_value) - 1);
> +	if (ret)
> +		return 0;

	If you don't get O2CB_ET_SERVICE_UNAVAILABLE, shouldn't you be
returning the error code?  An I/O error or internal failure shouldn't be
treated as "just try local mode," should it?

> diff --git a/libo2cb/o2cb_abi.h b/libo2cb/o2cb_abi.h
> index df25020..1edbf74 100644
> --- a/libo2cb/o2cb_abi.h
> +++ b/libo2cb/o2cb_abi.h
> @@ -39,5 +39,8 @@
>  #define O2CB_FORMAT_HEARTBEAT_DIR	O2CB_FORMAT_CLUSTER "/heartbeat"
>  #define O2CB_FORMAT_HEARTBEAT_REGION	O2CB_FORMAT_HEARTBEAT_DIR "/%s"
>  #define O2CB_FORMAT_HEARTBEAT_REGION_ATTR	O2CB_FORMAT_HEARTBEAT_REGION "/%s"
> +#define O2CB_FORMAT_HEARTBEAT_MODE	O2CB_FORMAT_HEARTBEAT_DIR "/mode"
> +
> +#define O2CB_GLOBAL_HEARTBEAT_TAG	"global"

	Why aren't you sharing the valid_hb_modes stuff?  I can't see a
good reason that "global" is defined in separate places.  Even if you
just put the "local" and "global" tags as #defines here in o2cb_abi.h
and then use those defines in valid_hb_modes in o2cb_config.c

Joel

-- 

"What do you take me for, an idiot?"  
        - General Charles de Gaulle, when a journalist asked him
          if he was happy.

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127