blktap2: Cleanup vdi stacking code.

Removes some rough edges, memory leakage (?), fixes __list_splice, ...

Signed-off-by: Jake Wires <jake.wires@citrix.com>
Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>

4 files changed, 116 insertions, 98 deletions

diff --git a/tools/blktap2/drivers/tapdisk-interface.c b/tools/blktap2/drivers/tapdisk-interface.c
index fedb17a3aa..2e518833db 100644
--- a/tools/blktap2/drivers/tapdisk-interface.c
+++ b/tools/blktap2/drivers/tapdisk-interface.c
@@ -59,7 +59,7 @@ td_load(td_image_t *image)
 }
 
 int
-td_open(td_image_t *image)
+__td_open(td_image_t *image, td_disk_info_t *info)
 {
 	int err;
 	td_driver_t *driver;
@@ -72,6 +72,9 @@ td_open(td_image_t *image)
 						 image->storage);
 		if (!driver)
 			return -ENOMEM;
+
+		if (info) /* pre-seed driver->info for virtual drivers */
+			driver->info = *info;
 	}
 
 	if (!td_flag_test(driver->state, TD_DRIVER_OPEN)) {
@@ -95,6 +98,12 @@ td_open(td_image_t *image)
 }
 
 int
+td_open(td_image_t *image)
+{
+	return __td_open(image, NULL);
+}
+
+int
 td_close(td_image_t *image)
 {
 	td_driver_t *driver;
diff --git a/tools/blktap2/drivers/tapdisk-interface.h b/tools/blktap2/drivers/tapdisk-interface.h
index 1e48e5811a..adc4376910 100644
--- a/tools/blktap2/drivers/tapdisk-interface.h
+++ b/tools/blktap2/drivers/tapdisk-interface.h
@@ -32,6 +32,7 @@
 #include "tapdisk-queue.h"
 
 int td_open(td_image_t *);
+int __td_open(td_image_t *, td_disk_info_t *);
 int td_load(td_image_t *);
 int td_close(td_image_t *);
 int td_get_parent_id(td_image_t *, td_disk_id_t *);
diff --git a/tools/blktap2/drivers/tapdisk-vbd.c b/tools/blktap2/drivers/tapdisk-vbd.c
index 65300acbf9..f39fdadb1a 100644
--- a/tools/blktap2/drivers/tapdisk-vbd.c
+++ b/tools/blktap2/drivers/tapdisk-vbd.c
@@ -82,6 +82,17 @@ tapdisk_vbd_initialize_vreq(td_vbd_request_t *vreq)
 	INIT_LIST_HEAD(&vreq->next);
 }
 
+void
+tapdisk_vbd_free(td_vbd_t *vbd)
+{
+	if (vbd) {
+		tapdisk_vbd_free_stack(vbd);
+		list_del_init(&vbd->next);
+		free(vbd->name);
+		free(vbd);
+	}
+}
+
 int
 tapdisk_vbd_initialize(uint16_t uuid)
 {
@@ -281,23 +292,21 @@ fail:
 	return err;
 }
 
-/* TODO: ugh, lets not call it parent info... */
-static struct list_head *
-tapdisk_vbd_open_level(td_vbd_t *vbd, const char* params, int driver_type, td_disk_info_t *parent_info, td_flag_t flags)
+static int
+tapdisk_vbd_open_level(td_vbd_t *vbd, struct list_head *head,
+		       const char *params, int driver_type,
+		       td_disk_info_t *driver_info, td_flag_t flags)
 {
 	const char *name;
 	int type, err;
 	td_image_t *image;
 	td_disk_id_t id;
-	struct  list_head *images;
 	td_driver_t *driver;
 
-	images = calloc(1, sizeof(struct list_head));
-	INIT_LIST_HEAD(images);
-
 	name    = params;
 	id.name = NULL;
 	type    = driver_type;
+	INIT_LIST_HEAD(head);
 
 	for (;;) {
 		err   = -ENOMEM;
@@ -307,42 +316,13 @@ tapdisk_vbd_open_level(td_vbd_t *vbd, const char* params, int driver_type, td_di
 		free(id.name);
 
 		if (!image)
-			return NULL;
-
+			goto out;
 
-		/* We have to do this to set the driver info for child drivers.  this conflicts with td_open */
-		driver = image->driver;
-		if (!driver) {
-			driver = tapdisk_driver_allocate(image->type,
-							 image->name,
-							 image->flags,
-							 image->storage);
-			if (!driver)
-				return NULL;
-		}
-		/* the image has a driver, set the info and driver */
-		image->driver = driver;
-		image->info = driver->info;
-
-		/* XXX: we don't touch driver->refcount, broken? */
-		/* XXX: we've replicated about 90% of td_open() gross! */
-		/* XXX: this breaks if a driver modifies its info within a layer */
-
-		/* if the parent info is set, pass it to the child */
-		if(parent_info)
-		{
-			image->driver->info = *parent_info;
-		}
-
-		err = td_load(image);
-		if (err) {
-			if (err != -ENODEV)
-				return NULL;
 
-			err = td_open(image);
-			if (err)
-				return NULL;
-		}
+		/* this breaks if a driver modifies its info within a layer */
+		err = __td_open(image, driver_info);
+		if (err)
+			goto out;
 
 		/* TODO: non-sink drivers that don't care about their child
 		 * currently return EINVAL. Could return TD_PARENT_OK or
@@ -351,60 +331,73 @@ tapdisk_vbd_open_level(td_vbd_t *vbd, const char* params, int driver_type, td_di
 		err = td_get_parent_id(image, &id);
 		if (err && (err != TD_NO_PARENT && err != -EINVAL)) {
 			td_close(image);
-			return NULL;
+			goto out;
 		}
 
-		if (!image->storage)
-			image->storage = vbd->storage;
-
 		/* add this image to the end of the list */
-		list_add_tail(&image->next, images);
-
+		list_add_tail(&image->next, head);
 		image = NULL;
 
 		/* if the image does not have a parent we return the
 		 * list of images generated by this level of the stack */
-		if (err == TD_NO_PARENT || err == -EINVAL)
-			break;
+		if (err == TD_NO_PARENT || err == -EINVAL) {
+			err = 0;
+			goto out;
+		}
 
 		name   = id.name;
 		type   = id.drivertype;
-#if 0
-		/* catch this by validate, not here */
+
 		flags |= (TD_OPEN_RDONLY | TD_OPEN_SHAREABLE);
-#endif
 	}
-	return images;
+
+out:
+	if (err) {
+		if (image) {
+			td_close(image);
+			tapdisk_image_free(image);
+		}
+		while (!list_empty(head)) {
+			image = list_entry(&head->next, td_image_t, next);
+			td_close(image);
+			tapdisk_image_free(image);
+		}
+	}
+
+	return err;
 }
 
 static int
 __tapdisk_vbd_open_vdi(td_vbd_t *vbd, td_flag_t extra_flags)
 {
-	const char *file;
-	int err, type;
+	int err;
 	td_flag_t flags;
-	td_disk_id_t id;
 	td_image_t *tmp;
-	struct tfilter *filter = NULL;
 	td_vbd_driver_info_t *driver_info;
 	struct list_head *images;
 	td_disk_info_t *parent_info = NULL;
 
+	if (list_empty(&vbd->driver_stack))
+		return -ENOENT;
+
 	flags = (vbd->flags & ~TD_OPEN_SHAREABLE) | extra_flags;
 
 	/* loop on each user specified driver.
 	 * NOTE: driver_info is in reverse order. That is, the first
 	 * item is the 'parent' or 'sink' driver */
 	list_for_each_entry(driver_info, &vbd->driver_stack, next) {
-		file = driver_info->params;
-		type = driver_info->type;
-		images = tapdisk_vbd_open_level(vbd, file, type, parent_info, flags);
-		if (!images)
-			return -EINVAL;
+		LIST_HEAD(images);
 
-		/* after each loop, append the created stack to the result stack */
-		list_splice(images, &vbd->images);
-		free(images);
+		err = tapdisk_vbd_open_level(vbd, &images,
+					     driver_info->params,
+					     driver_info->type,
+					     parent_info, flags);
+		if (err)
+			goto fail;
+
+		/* after each loop, 
+		 * append the created stack to the result stack */
+		list_splice(&images, &vbd->images);
 
 		/* set the parent_info to the first diskinfo on the stack */
 		tmp = tapdisk_vbd_first_image(vbd);
@@ -421,7 +414,7 @@ __tapdisk_vbd_open_vdi(td_vbd_t *vbd, td_flag_t extra_flags)
 		err = tapdisk_vbd_add_block_cache(vbd);
 		if (err)
 			goto fail;
-	}		
+	}
 
 	err = tapdisk_vbd_validate_chain(vbd);
 	if (err)
@@ -432,16 +425,7 @@ __tapdisk_vbd_open_vdi(td_vbd_t *vbd, td_flag_t extra_flags)
 	return 0;
 
 fail:
-
-/* TODO: loop over vbd to free images? maybe do that in vbd_close_vdi */
-#if 0
-	if (image)
-		tapdisk_image_free(image);
-#endif
-
-	/* TODO: handle partial stack creation? */
 	tapdisk_vbd_close_vdi(vbd);
-
 	return err;
 }
 
@@ -453,47 +437,71 @@ tapdisk_vbd_parse_stack(td_vbd_t *vbd, const char *path)
 	char *params, *driver_str;
 	td_vbd_driver_info_t *driver;
 
-	/* make a copy of path */
-	/* TODO: check against MAX_NAME_LEM ? */
 	err = tapdisk_namedup(&params, path);
-	if(err)
-		goto error;
-
+	if (err)
+		return err;
 
 	/* tokenize params based on pipe '|' */
 	driver_str = strtok(params, "|");
-	while(driver_str != NULL)
-	{
+	while (driver_str != NULL) {
+		const char *path;
+		int type;
+
 		/* parse driver info and add to vbd */
 		driver = calloc(1, sizeof(td_vbd_driver_info_t));
+		if (!driver) {
+			PERROR("malloc");
+			err = -errno;
+			goto out;
+		}
 		INIT_LIST_HEAD(&driver->next);
-		err = tapdisk_parse_disk_type(driver_str, &driver->params, &driver->type);
-		if(err)
-			goto error;
 
-		/* build the list backwards as the last driver will be the first
-		 * driver to open in the stack */
+		err = tapdisk_parse_disk_type(driver_str, &path, &type);
+		if (err) {
+			free(driver);
+			goto out;
+		}
+
+		driver->type   = type;
+		driver->params = strdup(path);
+		if (!driver->params) {
+			err = -ENOMEM;
+			free(driver);
+			goto out;
+		}
+
+		/* build the list backwards as the last driver will be the
+		 * first driver to open in the stack */
 		list_add(&driver->next, &vbd->driver_stack);
 
 		/* get next driver string */
 		driver_str = strtok(NULL, "|");
 	}
 
-	return 0;
+out:
+	free(params);
+	if (err)
+		tapdisk_vbd_free_stack(vbd);
+
+	return err;
+}
+
+void
+tapdisk_vbd_free_stack(td_vbd_t *vbd)
+{
+	td_vbd_driver_info_t *driver;
 
-	/* error: free any driver_info's and params */
- error:
-	while(!list_empty(&vbd->driver_stack)) {
-		driver = list_entry(vbd->driver_stack.next, td_vbd_driver_info_t, next);
+	while (!list_empty(&vbd->driver_stack)) {
+		driver = list_entry(vbd->driver_stack.next,
+				    td_vbd_driver_info_t, next);
 		list_del(&driver->next);
+		free(driver->params);
 		free(driver);
 	}
-
-	return err;
 }
 
 /* NOTE: driver type, etc. must be set */
-static int
+int
 tapdisk_vbd_open_stack(td_vbd_t *vbd, uint16_t storage, td_flag_t flags)
 {
 	int i, err;
@@ -536,7 +544,6 @@ tapdisk_vbd_open_vdi(td_vbd_t *vbd, const char *path,
 
 	vbd->flags   = flags;
 	vbd->storage = storage;
-	vbd->type    = drivertype;
 
 	for (i = 0; i < TD_VBD_EIO_RETRIES; i++) {
 		err = __tapdisk_vbd_open_vdi(vbd, 0);
diff --git a/tools/blktap2/drivers/tapdisk-vbd.h b/tools/blktap2/drivers/tapdisk-vbd.h
index 3d26942582..0fea53e4f9 100644
--- a/tools/blktap2/drivers/tapdisk-vbd.h
+++ b/tools/blktap2/drivers/tapdisk-vbd.h
@@ -80,7 +80,7 @@ struct td_vbd_request {
 };
 
 struct td_vbd_driver_info {
-	const char                 *params;
+	char                       *params;
 	int                         type;
 	struct list_head            next;
 };
@@ -174,6 +174,7 @@ int tapdisk_vbd_parse_stack(td_vbd_t *vbd, const char *path);
 int tapdisk_vbd_open(td_vbd_t *, const char *, uint16_t,
 		     uint16_t, const char *, td_flag_t);
 int tapdisk_vbd_close(td_vbd_t *);
+void tapdisk_vbd_free_stack(td_vbd_t *);
 
 int tapdisk_vbd_open_vdi(td_vbd_t *, const char *,
 			 uint16_t, uint16_t, td_flag_t);