Mailing List Archive

# HG changeset patch
# User Keir Fraser <keir.fraser@citrix.com>
# Date 1275980676 -3600
# Node ID 54675b91b3c17093efd347bd88ea57fce7240f30
# Parent a5032d7a87e020848acc4a9c349f92c4b4e3b0cf
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>
---
tools/blktap2/drivers/tapdisk-interface.c | 11 +
tools/blktap2/drivers/tapdisk-interface.h | 1
tools/blktap2/drivers/tapdisk-vbd.c | 211 +++++++++++++++---------------
tools/blktap2/drivers/tapdisk-vbd.h | 3
tools/blktap2/include/list.h | 19 +-
5 files changed, 132 insertions(+), 113 deletions(-)

diff -r a5032d7a87e0 -r 54675b91b3c1 tools/blktap2/drivers/tapdisk-interface.c
--- a/tools/blktap2/drivers/tapdisk-interface.c Tue Jun 08 08:04:09 2010 +0100
+++ b/tools/blktap2/drivers/tapdisk-interface.c Tue Jun 08 08:04:36 2010 +0100
@@ -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 -r a5032d7a87e0 -r 54675b91b3c1 tools/blktap2/drivers/tapdisk-interface.h
--- a/tools/blktap2/drivers/tapdisk-interface.h Tue Jun 08 08:04:09 2010 +0100
+++ b/tools/blktap2/drivers/tapdisk-interface.h Tue Jun 08 08:04:36 2010 +0100
@@ -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 -r a5032d7a87e0 -r 54675b91b3c1 tools/blktap2/drivers/tapdisk-vbd.c
--- a/tools/blktap2/drivers/tapdisk-vbd.c Tue Jun 08 08:04:09 2010 +0100
+++ b/tools/blktap2/drivers/tapdisk-vbd.c Tue Jun 08 08:04:36 2010 +0100
@@ -82,6 +82,17 @@ tapdisk_vbd_initialize_vreq(td_vbd_reque
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, co
free(id.name);

if (!image)
- return NULL;
-
-
- /* 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;
- }
+ goto out;
+
+
+ /* 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,44 +331,54 @@ tapdisk_vbd_open_level(td_vbd_t *vbd, co
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;

@@ -396,15 +386,18 @@ __tapdisk_vbd_open_vdi(td_vbd_t *vbd, td
* 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;
-
- /* after each loop, append the created stack to the result stack */
- list_splice(images, &vbd->images);
- free(images);
+ LIST_HEAD(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
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
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, c
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;
-
- /* 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);
+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;
+
+ 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, cons

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 -r a5032d7a87e0 -r 54675b91b3c1 tools/blktap2/drivers/tapdisk-vbd.h
--- a/tools/blktap2/drivers/tapdisk-vbd.h Tue Jun 08 08:04:09 2010 +0100
+++ b/tools/blktap2/drivers/tapdisk-vbd.h Tue Jun 08 08:04:36 2010 +0100
@@ -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_open(td_vbd_t *, const c
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);
diff -r a5032d7a87e0 -r 54675b91b3c1 tools/blktap2/include/list.h
--- a/tools/blktap2/include/list.h Tue Jun 08 08:04:09 2010 +0100
+++ b/tools/blktap2/include/list.h Tue Jun 08 08:04:36 2010 +0100
@@ -87,24 +87,25 @@ static inline int list_is_last(const str
return list->next == head;
}

-static inline void __list_splice(struct list_head *list,
- struct list_head *head)
+static inline void __list_splice(const struct list_head *list,
+ struct list_head *prev,
+ struct list_head *next)
{
struct list_head *first = list->next;
struct list_head *last = list->prev;
- struct list_head *at = head->next;

- first->prev = head;
- head->next = first;
+ first->prev = prev;
+ prev->next = first;

- last->next = at;
- at->prev = last;
+ last->next = next;
+ next->prev = last;
}

-static inline void list_splice(struct list_head *list, struct list_head *head)
+static inline void list_splice(const struct list_head *list,
+ struct list_head *head)
{
if (!list_empty(list))
- __list_splice(list, head);
+ __list_splice(list, head, head->next);
}

#define list_entry(ptr, type, member) \

_______________________________________________
Xen-changelog mailing list
Xen-changelog@lists.xensource.com
http://lists.xensource.com/xen-changelog