The IPC patch broke CFG shutdown in several places, this patches fixes

all of them.

In particular, cfg_try_shutdown asks all applications that are
registered for callbacks if they approve the shutdown. This caused a bit
of a re-entrancy problem because it also asked the process that called
for the shutdown! The patch causes cfg to only ask OTHER applications in
the assumption that any application that calls
corosync_cfg_tryshutdown() will approve of the action :-)

In addition it adds the response to cfg_replyto_shutdown which was
missing (it couldn't be used with the old system but is mandatory now),
and removes a double-free in the library finalise code.



git-svn-id: http://svn.fedorahosted.org/svn/corosync/trunk@1794 fd59a12c-fef9-0310-b244-a6a79926bd2f

include/corosync/ipc_cfg.h

@@ -69,7 +69,8 @@ enum res_lib_cfg_types {
 	MESSAGE_RES_CFG_TRYSHUTDOWN = 9,
 	MESSAGE_RES_CFG_TESTSHUTDOWN = 10,
 	MESSAGE_RES_CFG_GET_NODE_ADDRS = 11,
-	MESSAGE_RES_CFG_LOCAL_GET = 12
+	MESSAGE_RES_CFG_LOCAL_GET = 12,
+	MESSAGE_RES_CFG_REPLYTOSHUTDOWN = 13
 };
 
 struct req_lib_cfg_statetrack {

lib/cfg.c

@@ -295,8 +295,6 @@ corosync_cfg_finalize (
 
 	pthread_mutex_destroy (&cfg_instance->dispatch_mutex);
 
-	cslib_service_disconnect (&cfg_instance->ipc_ctx);
-
 	(void)saHandleDestroy (&cfg_hdb, cfg_handle);
 
 	(void)saHandleInstancePut (&cfg_hdb, cfg_handle);

services/cfg.c

@@ -237,8 +237,8 @@ static struct corosync_lib_handler cfg_lib_engine[] =
 	},
 	{ /* 10 */
 		.lib_handler_fn		= message_handler_req_lib_cfg_replytoshutdown,
-		.response_size		= 0,
-		.response_id		= 0,
+		.response_size		= sizeof (struct res_lib_cfg_replytoshutdown),
+		.response_id		= MESSAGE_RES_CFG_REPLYTOSHUTDOWN,
 		.flow_control		= CS_LIB_FLOW_CONTROL_NOT_REQUIRED
 	},
 	{ /* 11 */
@@ -385,7 +385,7 @@ static int send_shutdown()
 	return 0;
 }
 
-static void send_test_shutdown(void * conn, int status)
+static void send_test_shutdown(void *only_conn, void *exclude_conn, int status)
 {
 	struct res_lib_cfg_testshutdown res_lib_cfg_testshutdown;
 	struct list_head *iter;
@@ -396,17 +396,19 @@ static void send_test_shutdown(void * conn, int status)
 	res_lib_cfg_testshutdown.header.error = status;
 	res_lib_cfg_testshutdown.flags = shutdown_flags;
 
-	if (conn) {
-		TRACE1("sending testshutdown to %p", conn);
-		api->ipc_response_send(conn, &res_lib_cfg_testshutdown,
-					    sizeof(res_lib_cfg_testshutdown));
+	if (only_conn) {
+		TRACE1("sending testshutdown to only %p", only_conn);
+		api->ipc_dispatch_send(only_conn, &res_lib_cfg_testshutdown,
+				       sizeof(res_lib_cfg_testshutdown));
 	} else {
 		for (iter = trackers_list.next; iter != &trackers_list; iter = iter->next) {
 			struct cfg_info *ci = list_entry(iter, struct cfg_info, list);
 
-			TRACE1("sending testshutdown to %p", ci->tracker_conn);
-			api->ipc_dispatch_send(ci->tracker_conn, &res_lib_cfg_testshutdown,
-						    sizeof(res_lib_cfg_testshutdown));
+			if (ci->conn != exclude_conn) {
+				TRACE1("sending testshutdown to %p", ci->tracker_conn);
+				api->ipc_dispatch_send(ci->tracker_conn, &res_lib_cfg_testshutdown,
+						       sizeof(res_lib_cfg_testshutdown));
+			}
 		}
 	}
 	LEAVE();
@@ -436,11 +438,6 @@ static void check_shutdown_status()
 		    shutdown_flags == CFG_SHUTDOWN_FLAG_REGARDLESS) {
 			TRACE1("shutdown confirmed");
 
-			/*
-			 * Tell other nodes we are going down
-			 */
-			send_shutdown();
-
 			res_lib_cfg_tryshutdown.header.size = sizeof(struct res_lib_cfg_tryshutdown);
 			res_lib_cfg_tryshutdown.header.id = MESSAGE_RES_CFG_TRYSHUTDOWN;
 			res_lib_cfg_tryshutdown.header.error = CS_OK;
@@ -451,6 +448,12 @@ static void check_shutdown_status()
 			api->ipc_response_send(shutdown_con->conn, &res_lib_cfg_tryshutdown,
 						    sizeof(res_lib_cfg_tryshutdown));
 			shutdown_con = NULL;
+
+			/*
+			 * Tell other nodes we are going down
+			 */
+			send_shutdown();
+
 		}
 		else {
 
@@ -486,7 +489,7 @@ static void shutdown_timer_fn(void *arg)
 	shutdown_no = shutdown_expected;
 	check_shutdown_status();
 
-	send_test_shutdown(NULL, CS_ERR_TIMEOUT);
+	send_test_shutdown(NULL, NULL, CS_ERR_TIMEOUT);
 	LEAVE();
 }
 
@@ -695,7 +698,6 @@ static void message_handler_req_lib_cfg_statetrack (
 	void *msg)
 {
 	struct cfg_info *ci = (struct cfg_info *)api->ipc_private_data_get (conn);
-//	struct req_lib_cfg_statetrack *req_lib_cfg_statetrack = (struct req_lib_cfg_statetrack *)message;
 	struct res_lib_cfg_statetrack res_lib_cfg_statetrack;
 
 	ENTER();
@@ -713,7 +715,7 @@ static void message_handler_req_lib_cfg_statetrack (
 			 */
 			ci->shutdown_reply = SHUTDOWN_REPLY_UNKNOWN;
 			shutdown_expected++;
-			send_test_shutdown(conn, CS_OK);
+			send_test_shutdown(conn, NULL, CS_OK);
 		}
 	}
 
@@ -898,15 +900,32 @@ static void message_handler_req_lib_cfg_tryshutdown (
 	shutdown_expected = 0;
 
 	for (iter = trackers_list.next; iter != &trackers_list; iter = iter->next) {
-		struct cfg_info *ci = list_entry(iter, struct cfg_info, list);
-		ci->shutdown_reply = SHUTDOWN_REPLY_UNKNOWN;
-		shutdown_expected++;
+		struct cfg_info *testci = list_entry(iter, struct cfg_info, list);
+		/*
+		 * It is assumed that we will allow shutdown
+		 */
+		if (testci != ci) {
+			testci->shutdown_reply = SHUTDOWN_REPLY_UNKNOWN;
+			shutdown_expected++;
+		}
 	}
 
 	/*
 	 * If no-one is listening for events then we can just go down now
 	 */
 	if (shutdown_expected == 0) {
+		struct res_lib_cfg_tryshutdown res_lib_cfg_tryshutdown;
+
+		res_lib_cfg_tryshutdown.header.size = sizeof(struct res_lib_cfg_tryshutdown);
+		res_lib_cfg_tryshutdown.header.id = MESSAGE_RES_CFG_TRYSHUTDOWN;
+		res_lib_cfg_tryshutdown.header.error = CS_OK;
+
+		/*
+		 * Tell originator that shutdown was confirmed
+		 */
+		api->ipc_response_send(conn, &res_lib_cfg_tryshutdown,
+				       sizeof(res_lib_cfg_tryshutdown));
+
 		send_shutdown();
 		LEAVE();
 		return;
@@ -944,7 +963,7 @@ static void message_handler_req_lib_cfg_tryshutdown (
 		/*
 		 * Tell the users we would like to shut down
 		 */
-		send_test_shutdown(NULL, CS_OK);
+		send_test_shutdown(NULL, conn, CS_OK);
 	}
 
 	/*
@@ -961,11 +980,13 @@ static void message_handler_req_lib_cfg_replytoshutdown (
 {
 	struct cfg_info *ci = (struct cfg_info *)api->ipc_private_data_get (conn);
 	struct req_lib_cfg_replytoshutdown *req_lib_cfg_replytoshutdown = (struct req_lib_cfg_replytoshutdown *)msg;
+	struct res_lib_cfg_replytoshutdown res_lib_cfg_replytoshutdown;
+	int status = CS_OK;
 
 	ENTER();
 	if (!shutdown_con) {
-		LEAVE();
-		return;
+		status = CS_ERR_ACCESS;
+		goto exit_fn;
 	}
 
 	if (req_lib_cfg_replytoshutdown->response) {
@@ -977,6 +998,15 @@ static void message_handler_req_lib_cfg_replytoshutdown (
 		ci->shutdown_reply = SHUTDOWN_REPLY_NO;
 	}
 	check_shutdown_status();
+
+exit_fn:
+	res_lib_cfg_replytoshutdown.header.error = status;
+	res_lib_cfg_replytoshutdown.header.id = MESSAGE_RES_CFG_REPLYTOSHUTDOWN;
+	res_lib_cfg_replytoshutdown.header.size = sizeof(res_lib_cfg_replytoshutdown);
+
+	api->ipc_response_send(conn, &res_lib_cfg_replytoshutdown,
+			       sizeof(res_lib_cfg_replytoshutdown));
+
 	LEAVE();
 }
 

tools/corosync-cfgtool.c

@@ -147,36 +147,13 @@ void service_unload_do (char *service, unsigned int version)
 	(void)corosync_cfg_finalize (handle);
 }
 
-void shutdown_callback (corosync_cfg_handle_t cfg_handle, corosync_cfg_shutdown_flags_t flags)
-{
-	printf("shutdown callback called, flags = %d\n",flags);
-
-	(void)corosync_cfg_replyto_shutdown (cfg_handle, COROSYNC_CFG_SHUTDOWN_FLAG_YES);
-}
-
-void *shutdown_dispatch_thread(void *arg)
-{
-	int res = CS_OK;
-	corosync_cfg_handle_t *handle = arg;
-
-	while (res == CS_OK) {
-		res = corosync_cfg_dispatch(*handle, CS_DISPATCH_ALL);
-		if (res != CS_OK)
-			printf ("Could not dispatch cfg messages: %d\n", res);
-	}
-	return NULL;
-}
-
 void shutdown_do()
 {
 	cs_error_t result;
 	corosync_cfg_handle_t handle;
 	corosync_cfg_callbacks_t callbacks;
-	corosync_cfg_state_notification_t notification_buffer;
-	pthread_t dispatch_thread;
 
-	printf ("Shutting down corosync\n");
-	callbacks.corosync_cfg_shutdown_callback = shutdown_callback;
+	callbacks.corosync_cfg_shutdown_callback = NULL;
 
 	result = corosync_cfg_initialize (&handle, &callbacks);
 	if (result != CS_OK) {
@@ -184,16 +161,7 @@ void shutdown_do()
 		exit (1);
 	}
 
-	pthread_create(&dispatch_thread, NULL, shutdown_dispatch_thread, &handle);
-
-	result = corosync_cfg_state_track (handle,
-					   0,
-					   &notification_buffer);
-	if (result != CS_OK) {
-		printf ("Could not start corosync cfg tracking error %d\n", result);
-		exit (1);
-	}
-
+	printf ("Shutting down corosync\n");
 	result = corosync_cfg_try_shutdown (handle, COROSYNC_CFG_SHUTDOWN_FLAG_REQUEST);
 	if (result != CS_OK) {
 		printf ("Could not shutdown (error = %d)\n", result);
@@ -262,7 +230,7 @@ void killnode_do(unsigned int nodeid)
 
 void usage_do (void)
 {
-	printf ("corosync-cfgtool [-s] [-r] [-l] [-u] [service_name] [-v] [version] [-k] [nodeid] [-a] [nodeid]\n\n");
+	printf ("corosync-cfgtool [-s] [-r] [-l] [-u] [-H] [service_name] [-v] [version] [-k] [nodeid] [-a] [nodeid]\n\n");
 	printf ("A tool for displaying and configuring active parameters within corosync.\n");
 	printf ("options:\n");
 	printf ("\t-s\tDisplays the status of the current rings on this node.\n");
@@ -272,11 +240,11 @@ void usage_do (void)
 	printf ("\t-u\tUnload a service identified by name.\n");
 	printf ("\t-a\tDisplay the IP address(es) of a node\n");
 	printf ("\t-k\tKill a node identified by node id.\n");
-	printf ("\t-h\tShutdown corosync cleanly on this node.\n");
+	printf ("\t-H\tShutdown corosync cleanly on this node.\n");
 }
 
 int main (int argc, char *argv[]) {
-	const char *options = "srl:u:v:k:a:h";
+	const char *options = "srl:u:v:k:a:hH";
 	int opt;
 	int service_load = 0;
 	unsigned int nodeid;
@@ -307,7 +275,7 @@ int main (int argc, char *argv[]) {
 			nodeid = atoi (optarg);
 			killnode_do(nodeid);
 			break;
-		case 'h':
+		case 'H':
 			shutdown_do();
 			break;
 		case 'a':
@@ -316,6 +284,9 @@ int main (int argc, char *argv[]) {
 		case 'v':
 			version = atoi (optarg);
 			break;
+		case 'h':
+			usage_do();
+			break;
 		}
 	}