[RFC Patch v3 12/18] block-remus: fix bug in tdremus_close()

[Wen Congyang <wency@cn.fujitsu.com>]

We close ctl_fd.fd, but we don't unregister ctl_fd.id. It will
cause select() return fails, and the user cannot talk with
tapdisk2.

This patch also does some cleanup.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
---
 tools/blktap2/drivers/block-remus.c | 90 ++++++++++++++++++++++---------------
 1 file changed, 53 insertions(+), 37 deletions(-)

diff --git a/tools/blktap2/drivers/block-remus.c b/tools/blktap2/drivers/block-remus.c
index 23a908a..55363a3 100644
--- a/tools/blktap2/drivers/block-remus.c
+++ b/tools/blktap2/drivers/block-remus.c
@@ -151,9 +151,6 @@ typedef struct poll_fd {
 } poll_fd_t;
 
 struct tdremus_state {
-//  struct tap_disk* driver;
-	void* driver_data;
-
   /* XXX: this is needed so that the server can perform operations on
    * the driver from the stream_fd event handler. fix this. */
 	td_driver_t *tdremus_driver;
@@ -731,12 +728,26 @@ static int mwrite(int fd, void* buf, size_t len)
 
 static void inline close_stream_fd(struct tdremus_state *s)
 {
+	if (s->stream_fd.fd < 0)
+		return;
+
 	/* XXX: -2 is magic. replace with macro perhaps? */
 	tapdisk_server_unregister_event(s->stream_fd.id);
 	close(s->stream_fd.fd);
 	s->stream_fd.fd = -2;
 }
 
+static void close_server_fd(struct tdremus_state *s)
+{
+	if (s->server_fd.fd < 0)
+		return;
+
+	tapdisk_server_unregister_event(s->server_fd.id);
+	s->server_fd.id = -1;
+	close(s->stream_fd.fd);
+	s->stream_fd.fd = -1;
+}
+
 /* primary functions */
 static void remus_client_event(event_id_t, char mode, void *private);
 static void remus_connect_event(event_id_t id, char mode, void *private);
@@ -1347,12 +1358,7 @@ static int unprotected_start(td_driver_t *driver)
 	/* close the server socket */
 	close_stream_fd(s);
 
-	/* unregister the replication stream */
-	tapdisk_server_unregister_event(s->server_fd.id);
-
-	/* close the replication stream */
-	close(s->server_fd.fd);
-	s->server_fd.fd = -1;
+	close_server_fd(s);
 
 	/* install the unprotected read/write handlers */
 	tapdisk_remus.td_queue_read = unprotected_queue_read;
@@ -1553,27 +1559,27 @@ static int ctl_open(td_driver_t *driver, const char* name)
 			s->ctl_path[i] = '_';
 	}
 	if (asprintf(&s->msg_path, "%s.msg", s->ctl_path) < 0)
-		goto err_ctlfifo;
+		goto err_setmsgfifo;
 
 	if (mkfifo(s->ctl_path, S_IRWXU|S_IRWXG|S_IRWXO) && errno != EEXIST) {
 		RPRINTF("error creating control FIFO %s: %d\n", s->ctl_path, errno);
-		goto err_msgfifo;
+		goto err_mkctlfifo;
 	}
 
 	if (mkfifo(s->msg_path, S_IRWXU|S_IRWXG|S_IRWXO) && errno != EEXIST) {
 		RPRINTF("error creating message FIFO %s: %d\n", s->msg_path, errno);
-		goto err_msgfifo;
+		goto err_mkmsgfifo;
 	}
 
 	/* RDWR so that fd doesn't block select when no writer is present */
 	if ((s->ctl_fd.fd = open(s->ctl_path, O_RDWR)) < 0) {
 		RPRINTF("error opening control FIFO %s: %d\n", s->ctl_path, errno);
-		goto err_msgfifo;
+		goto err_openctlfifo;
 	}
 
 	if ((s->msg_fd.fd = open(s->msg_path, O_RDWR)) < 0) {
 		RPRINTF("error opening message FIFO %s: %d\n", s->msg_path, errno);
-		goto err_openctlfifo;
+		goto err_openmsgfifo;
 	}
 
 	RPRINTF("control FIFO %s\n", s->ctl_path);
@@ -1581,36 +1587,45 @@ static int ctl_open(td_driver_t *driver, const char* name)
 
 	return 0;
 
- err_openctlfifo:
+err_openmsgfifo:
 	close(s->ctl_fd.fd);
- err_msgfifo:
+	s->ctl_fd.fd = -1;
+err_openctlfifo:
+	unlink(s->ctl_path);
+err_mkmsgfifo:
+	unlink(s->msg_path);
+err_mkctlfifo:
 	free(s->msg_path);
 	s->msg_path = NULL;
- err_ctlfifo:
+err_setmsgfifo:
 	free(s->ctl_path);
 	s->ctl_path = NULL;
 	return -1;
 }
 
-static void ctl_close(td_driver_t *driver)
+static void ctl_close(struct tdremus_state *s)
 {
-	struct tdremus_state *s = (struct tdremus_state *)driver->data;
-
-	/* TODO: close *all* connections */
-
-	if(s->ctl_fd.fd)
+	if(s->ctl_fd.fd) {
 		close(s->ctl_fd.fd);
+		s->ctl_fd.fd = -1;
+	}
 
 	if (s->ctl_path) {
 		unlink(s->ctl_path);
 		free(s->ctl_path);
 		s->ctl_path = NULL;
 	}
+
 	if (s->msg_path) {
 		unlink(s->msg_path);
 		free(s->msg_path);
 		s->msg_path = NULL;
 	}
+
+	if (s->msg_fd.fd) {
+		close(s->msg_fd.fd);
+		s->msg_fd.fd = -1;
+	}
 }
 
 static int ctl_register(struct tdremus_state *s)
@@ -1628,6 +1643,16 @@ static int ctl_register(struct tdremus_state *s)
 	return 0;
 }
 
+static void ctl_unregister(struct tdremus_state *s)
+{
+	RPRINTF("unregistering ctl fifo\n");
+
+	if (s->ctl_fd.id >= 0) {
+		tapdisk_server_unregister_event(s->ctl_fd.id);
+		s->ctl_fd.id = -1;
+	}
+}
+
 /* interface */
 
 static int tdremus_open(td_driver_t *driver, td_image_t *image, td_uuid_t uuid)
@@ -1658,13 +1683,12 @@ static int tdremus_open(td_driver_t *driver, td_image_t *image, td_uuid_t uuid)
 
 	if ((rc = ctl_open(driver, name))) {
 		RPRINTF("error setting up control channel\n");
-		free(s->driver_data);
 		return rc;
 	}
 
 	if ((rc = ctl_register(s))) {
 		RPRINTF("error registering control channel\n");
-		free(s->driver_data);
+		ctl_close(s);
 		return rc;
 	}
 
@@ -1687,19 +1711,11 @@ static int tdremus_close(td_driver_t *driver)
 	RPRINTF("closing\n");
 	if (s->ramdisk.inprogress)
 		hashtable_destroy(s->ramdisk.inprogress, 0);
-	
-	if (s->driver_data) {
-		free(s->driver_data);
-		s->driver_data = NULL;
-	}
-	if (s->server_fd.fd >= 0) {
-		close(s->server_fd.fd);
-		s->server_fd.fd = -1;
-	}
-	if (s->stream_fd.fd >= 0)
-		close_stream_fd(s);
 
-	ctl_close(driver);
+	close_server_fd(s);
+	close_stream_fd(s);
+	ctl_unregister(s);
+	ctl_close(s);
 
 	return 0;
 }
-- 
1.9.3