cfg: Improve nodestatusget versioning

Patch tries to make nodestatusget really extendable. Following changes
are implemented:
- corosync_cfg_node_status_version_t is added with (for now) single
  value CFG_NODE_STATUS_V1
- corosync_knet_node_status renamed to corosync_cfg_node_status_v1 (it
  isn't really knet because it works as well for udp(u()
- struct res_lib_cfg_nodestatusget_version is added which holds only ipc
  result header and version on same position as for
  corosync_cfg_node_status_v1
- corosync_cfg_node_status_get requires version and pointer to one of
  corosync_cfg_node_status_v structures
- request is handled in case switches to make adding new version easier

Also fix following bugs:
- totempg_nodestatus_get error was retyped to cs_error_t without any
  meaning.
- header.error was not checked at all in the library

Signed-off-by: Jan Friesse <jfriesse@redhat.com>
Reviewed-by: Christine Caulfield <ccaulfie@redhat.com>

exec/cfg.c

@@ -971,53 +971,76 @@ static void message_handler_req_lib_cfg_nodestatusget (
 	void *conn,
 	const void *msg)
 {
-	struct res_lib_cfg_nodestatusget res_lib_cfg_nodestatusget;
+	struct res_lib_cfg_nodestatusget_version res_lib_cfg_nodestatusget_version;
+	struct res_lib_cfg_nodestatusget_v1 res_lib_cfg_nodestatusget_v1;
+	void *res_lib_cfg_nodestatusget_ptr = NULL;
+	size_t res_lib_cfg_nodestatusget_size;
 	struct req_lib_cfg_nodestatusget *req_lib_cfg_nodestatusget = (struct req_lib_cfg_nodestatusget *)msg;
 	struct totem_node_status node_status;
-	cs_error_t res = CS_OK;
 	int i;
 
 	ENTER();
 
-	/* Currently only one structure version supported */
-	if (req_lib_cfg_nodestatusget->version == TOTEM_NODE_STATUS_STRUCTURE_VERSION)
-	{
-		res_lib_cfg_nodestatusget.header.id = MESSAGE_RES_CFG_NODESTATUSGET;
-		res_lib_cfg_nodestatusget.header.size = sizeof (struct res_lib_cfg_nodestatusget);
+	memset(&node_status, 0, sizeof(node_status));
+	if (totempg_nodestatus_get(req_lib_cfg_nodestatusget->nodeid, &node_status) != 0) {
+		res_lib_cfg_nodestatusget_ptr = &res_lib_cfg_nodestatusget_version;
+		res_lib_cfg_nodestatusget_size = sizeof(res_lib_cfg_nodestatusget_version);
 
-		memset(&node_status, 0, sizeof(node_status));
-		res = totempg_nodestatus_get(req_lib_cfg_nodestatusget->nodeid,
-				       &node_status);
-		if (res == 0) {
-			res_lib_cfg_nodestatusget.node_status.nodeid = req_lib_cfg_nodestatusget->nodeid;
-			res_lib_cfg_nodestatusget.node_status.version = node_status.version;
-			res_lib_cfg_nodestatusget.node_status.reachable = node_status.reachable;
-			res_lib_cfg_nodestatusget.node_status.remote = node_status.remote;
-			res_lib_cfg_nodestatusget.node_status.external = node_status.external;
-			res_lib_cfg_nodestatusget.node_status.onwire_min = node_status.onwire_min;
-			res_lib_cfg_nodestatusget.node_status.onwire_max = node_status.onwire_max;
-			res_lib_cfg_nodestatusget.node_status.onwire_ver= node_status.onwire_ver;
+		res_lib_cfg_nodestatusget_version.header.error = CS_ERR_FAILED_OPERATION;
+		res_lib_cfg_nodestatusget_version.header.id = MESSAGE_RES_CFG_NODESTATUSGET;
+		res_lib_cfg_nodestatusget_version.header.size = res_lib_cfg_nodestatusget_size;
 
-			for (i=0; i < KNET_MAX_LINK; i++) {
-				res_lib_cfg_nodestatusget.node_status.link_status[i].enabled = node_status.link_status[i].enabled;
-				res_lib_cfg_nodestatusget.node_status.link_status[i].connected = node_status.link_status[i].connected;
-				res_lib_cfg_nodestatusget.node_status.link_status[i].dynconnected = node_status.link_status[i].dynconnected;
-				res_lib_cfg_nodestatusget.node_status.link_status[i].mtu = node_status.link_status[i].mtu;
-				memcpy(res_lib_cfg_nodestatusget.node_status.link_status[i].src_ipaddr,
-				       node_status.link_status[i].src_ipaddr, CFG_MAX_HOST_LEN);
-				memcpy(res_lib_cfg_nodestatusget.node_status.link_status[i].dst_ipaddr,
-				       node_status.link_status[i].dst_ipaddr, CFG_MAX_HOST_LEN);
-			}
-		}
-	} else {
-		res = CS_ERR_NOT_SUPPORTED;
+		goto ipc_response_send;
 	}
 
-	res_lib_cfg_nodestatusget.header.error = res;
+	/* Currently only one structure version supported */
+	switch (req_lib_cfg_nodestatusget->version) {
+	case CFG_NODE_STATUS_V1:
+		res_lib_cfg_nodestatusget_ptr = &res_lib_cfg_nodestatusget_v1;
+		res_lib_cfg_nodestatusget_size = sizeof(res_lib_cfg_nodestatusget_v1);
+
+		res_lib_cfg_nodestatusget_v1.header.error = CS_OK;
+		res_lib_cfg_nodestatusget_v1.header.id = MESSAGE_RES_CFG_NODESTATUSGET;
+		res_lib_cfg_nodestatusget_v1.header.size = res_lib_cfg_nodestatusget_size;
+
+		res_lib_cfg_nodestatusget_v1.node_status.version = CFG_NODE_STATUS_V1;
+		res_lib_cfg_nodestatusget_v1.node_status.nodeid = req_lib_cfg_nodestatusget->nodeid;
+		res_lib_cfg_nodestatusget_v1.node_status.reachable = node_status.reachable;
+		res_lib_cfg_nodestatusget_v1.node_status.remote = node_status.remote;
+		res_lib_cfg_nodestatusget_v1.node_status.external = node_status.external;
+		res_lib_cfg_nodestatusget_v1.node_status.onwire_min = node_status.onwire_min;
+		res_lib_cfg_nodestatusget_v1.node_status.onwire_max = node_status.onwire_max;
+		res_lib_cfg_nodestatusget_v1.node_status.onwire_ver = node_status.onwire_ver;
+
+		for (i=0; i < KNET_MAX_LINK; i++) {
+			res_lib_cfg_nodestatusget_v1.node_status.link_status[i].enabled = node_status.link_status[i].enabled;
+			res_lib_cfg_nodestatusget_v1.node_status.link_status[i].connected = node_status.link_status[i].connected;
+			res_lib_cfg_nodestatusget_v1.node_status.link_status[i].dynconnected = node_status.link_status[i].dynconnected;
+			res_lib_cfg_nodestatusget_v1.node_status.link_status[i].mtu = node_status.link_status[i].mtu;
+			memcpy(res_lib_cfg_nodestatusget_v1.node_status.link_status[i].src_ipaddr,
+			       node_status.link_status[i].src_ipaddr, CFG_MAX_HOST_LEN);
+			memcpy(res_lib_cfg_nodestatusget_v1.node_status.link_status[i].dst_ipaddr,
+			       node_status.link_status[i].dst_ipaddr, CFG_MAX_HOST_LEN);
+		}
+		break;
+	default:
+		/*
+		 * Unsupported version requested
+		 */
+		res_lib_cfg_nodestatusget_ptr = &res_lib_cfg_nodestatusget_version;
+		res_lib_cfg_nodestatusget_size = sizeof(res_lib_cfg_nodestatusget_version);
+
+		res_lib_cfg_nodestatusget_version.header.error = CS_ERR_NOT_SUPPORTED;
+		res_lib_cfg_nodestatusget_version.header.id = MESSAGE_RES_CFG_NODESTATUSGET;
+		res_lib_cfg_nodestatusget_version.header.size = res_lib_cfg_nodestatusget_size;
+		break;
+	}
+
+ipc_response_send:
 	api->ipc_response_send (
 		conn,
-		&res_lib_cfg_nodestatusget,
-		sizeof (struct res_lib_cfg_nodestatusget));
+		res_lib_cfg_nodestatusget_ptr,
+		res_lib_cfg_nodestatusget_size);
 
 	LEAVE();
 }

include/corosync/cfg.h

@@ -162,10 +162,14 @@ corosync_cfg_ring_status_get (
 	char ***status,
 	unsigned int *interface_count);
 
-#define CFG_NODE_STATUS_STRUCT_VERSION 1
+typedef enum {
+	CFG_NODE_STATUS_V1 = 1,
+} corosync_cfg_node_status_version_t;
+
 #define CFG_MAX_HOST_LEN 256
 #define CFG_MAX_LINKS 8
-struct corosync_knet_link_status {
+
+struct corosync_knet_link_status_v1 {
 	uint8_t enabled;	        /* link is configured and admin enabled for traffic */
 	uint8_t connected;              /* link is connected for data (local view) */
 	uint8_t dynconnected;	        /* link has been activated by remote dynip */
@@ -174,8 +178,8 @@ struct corosync_knet_link_status {
 	char dst_ipaddr[CFG_MAX_HOST_LEN];
 };
 
-struct corosync_knet_node_status {
-        uint32_t version;
+struct corosync_cfg_node_status_v1 {
+	corosync_cfg_node_status_version_t version;
 	unsigned int nodeid;
 	uint8_t reachable;
 	uint8_t remote;
@@ -183,7 +187,7 @@ struct corosync_knet_node_status {
 	uint8_t onwire_min;
 	uint8_t onwire_max;
 	uint8_t onwire_ver;
-	struct corosync_knet_link_status link_status[CFG_MAX_LINKS];
+	struct corosync_knet_link_status_v1 link_status[CFG_MAX_LINKS];
 };
 
 /**
@@ -197,7 +201,8 @@ cs_error_t
 corosync_cfg_node_status_get (
 	corosync_cfg_handle_t cfg_handle,
 	unsigned int nodeid,
-	struct corosync_knet_node_status *node_status);
+	corosync_cfg_node_status_version_t version,
+	void *node_status);
 
 /**
  * @brief corosync_cfg_kill_node

include/corosync/ipc_cfg.h

@@ -112,12 +112,17 @@ struct req_lib_cfg_nodestatusget {
 	mar_uint32_t version __attribute__((aligned(8)));
 };
 
+struct res_lib_cfg_nodestatusget_version {
+	struct qb_ipc_response_header header __attribute__((aligned(8)));
+	corosync_cfg_node_status_version_t version __attribute__((aligned(8)));
+};
+
 /**
  * @brief The res_lib_cfg_nodestatusget struct
  */
-struct res_lib_cfg_nodestatusget {
+struct res_lib_cfg_nodestatusget_v1 {
 	struct qb_ipc_response_header header __attribute__((aligned(8)));
-	struct corosync_knet_node_status node_status __attribute__((aligned(8)));
+	struct corosync_cfg_node_status_v1 node_status __attribute__((aligned(8)));
 };
 
 /**

lib/cfg.c

@@ -371,18 +371,33 @@ cs_error_t
 corosync_cfg_node_status_get (
 	corosync_cfg_handle_t cfg_handle,
 	unsigned int nodeid,
-	struct corosync_knet_node_status *node_status)
+	corosync_cfg_node_status_version_t version,
+	void *node_status)
 {
 	struct cfg_inst *cfg_inst;
 	struct req_lib_cfg_nodestatusget req_lib_cfg_nodestatusget;
-	struct res_lib_cfg_nodestatusget res_lib_cfg_nodestatusget;
 	cs_error_t error;
 	struct iovec iov;
+	size_t cfg_node_status_size;
+	void *res_lib_cfg_nodestatuget_ptr;
+	struct res_lib_cfg_nodestatusget_v1 res_lib_cfg_nodestatusget_v1;
+	struct res_lib_cfg_nodestatusget_version *res_lib_cfg_nodestatusget_version;
 
 	if (!node_status) {
 		return (CS_ERR_INVALID_PARAM);
 	}
 
+	switch (version) {
+	case CFG_NODE_STATUS_V1:
+		cfg_node_status_size = sizeof(struct res_lib_cfg_nodestatusget_v1);
+		res_lib_cfg_nodestatuget_ptr = &res_lib_cfg_nodestatusget_v1;
+
+		break;
+	default:
+		return (CS_ERR_INVALID_PARAM);
+		break;
+	}
+
 	error = hdb_error_to_cs(hdb_handle_get (&cfg_hdb, cfg_handle, (void *)&cfg_inst));
 	if (error != CS_OK) {
 		return (error);
@@ -391,7 +406,7 @@ corosync_cfg_node_status_get (
 	req_lib_cfg_nodestatusget.header.size = sizeof (struct req_lib_cfg_nodestatusget);
 	req_lib_cfg_nodestatusget.header.id = MESSAGE_REQ_CFG_NODESTATUSGET;
 	req_lib_cfg_nodestatusget.nodeid = nodeid;
-	req_lib_cfg_nodestatusget.version = CFG_NODE_STATUS_STRUCT_VERSION;
+	req_lib_cfg_nodestatusget.version = version;
 
 	iov.iov_base = (void *)&req_lib_cfg_nodestatusget,
 	iov.iov_len = sizeof (struct req_lib_cfg_nodestatusget),
@@ -399,19 +414,34 @@ corosync_cfg_node_status_get (
 	error = qb_to_cs_error (qb_ipcc_sendv_recv(cfg_inst->c,
 		&iov,
 		1,
-		&res_lib_cfg_nodestatusget,
-		sizeof (struct res_lib_cfg_nodestatusget), CS_IPC_TIMEOUT_MS));
-
-	if (error == CS_OK) {
-		memcpy(node_status, &res_lib_cfg_nodestatusget.node_status, sizeof(struct corosync_knet_node_status));
+		res_lib_cfg_nodestatuget_ptr,
+		cfg_node_status_size, CS_IPC_TIMEOUT_MS));
+	if (error != CS_OK) {
+		goto error_put;
 	}
 
-	/* corosync sent us something we don't really understand.
-	   - we might need to revisit this in the case of future structure versions */
-	if (res_lib_cfg_nodestatusget.node_status.version != CFG_NODE_STATUS_STRUCT_VERSION) {
+	res_lib_cfg_nodestatusget_version = res_lib_cfg_nodestatuget_ptr;
+	error = res_lib_cfg_nodestatusget_version->header.error;
+	if (error != CS_OK) {
+		goto error_put;
+	}
+
+	if (res_lib_cfg_nodestatusget_version->version != version) {
+		/*
+		 * corosync sent us something we don't really understand.
+		 */
 		error = CS_ERR_NOT_SUPPORTED;
+		goto error_put;
 	}
 
+	switch (version) {
+	case CFG_NODE_STATUS_V1:
+		memcpy(node_status, &res_lib_cfg_nodestatusget_v1.node_status,
+		    sizeof(struct corosync_cfg_node_status_v1));
+		break;
+	}
+
+error_put:
 	(void)hdb_handle_put (&cfg_hdb, cfg_handle);
 
 	return (error);

tools/corosync-cfgtool.c

@@ -111,7 +111,7 @@ nodestatusget_do (enum user_action action, int brief)
 	int rc = EXIT_SUCCESS;
 	int transport_number = TOTEM_TRANSPORT_KNET;
 	int i,j;
-	struct corosync_knet_node_status node_status;
+	struct corosync_cfg_node_status_v1 node_status;
 
 	result = corosync_cfg_initialize (&handle, NULL);
 	if (result != CS_OK) {
@@ -191,7 +191,7 @@ nodestatusget_do (enum user_action action, int brief)
         /* If node status requested then do print node-based info */
 	if (action == ACTION_NODESTATUS_GET) {
 		for (i=0; i<s; i++) {
-			result = corosync_cfg_node_status_get(handle, nodeid_list[i], &node_status);
+			result = corosync_cfg_node_status_get(handle, nodeid_list[i], CFG_NODE_STATUS_V1, &node_status);
 			if (result == CS_OK) {
 				/* Only display node info if it is reachable (and not us) */
 				if (node_status.reachable && node_status.nodeid != local_nodeid) {
@@ -238,11 +238,11 @@ nodestatusget_do (enum user_action action, int brief)
 	}
 	/* Print in link order */
 	else {
-		struct corosync_knet_node_status node_info[s];
+		struct corosync_cfg_node_status_v1 node_info[s];
 		memset(node_info, 0, sizeof(node_info));
 
 		for (i=0; i<s; i++) {
-			result = corosync_cfg_node_status_get(handle, nodeid_list[i], &node_info[i]);
+			result = corosync_cfg_node_status_get(handle, nodeid_list[i], CFG_NODE_STATUS_V1, &node_info[i]);
 			if (result != CS_OK) {
 				fprintf (stderr, "Could not get the node status for nodeid %d, the error is: %d\n", nodeid_list[i], result);
 			}