[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Xen-changelog] [xen-unstable] blktap2: Cleanup vdi stacking code.



# HG changeset patch
# User Keir Fraser <keir.fraser@xxxxxxxxxx>
# 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@xxxxxxxxxx>
Signed-off-by: Daniel Stodden <daniel.stodden@xxxxxxxxxx>
---
 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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-changelog


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.