# HG changeset patch # User Daniel Stodden # Date 1275947595 25200 # Node ID 3ea1751e3fe79c53acec392ee060b34491925235 # Parent d8393489b4c4d93944203f62578f8cf2721ce961 blktap2: Cleanup vdi stacking code. Removes some rough edges, memory leakage (?), fixes __list_splice, ... Signed-off-by: Jake Wires Signed-off-by: Daniel Stodden diff -r d8393489b4c4 -r 3ea1751e3fe7 tools/blktap2/drivers/tapdisk-interface.c --- a/tools/blktap2/drivers/tapdisk-interface.c Mon Jun 07 14:53:15 2010 -0700 +++ b/tools/blktap2/drivers/tapdisk-interface.c Mon Jun 07 14:53:15 2010 -0700 @@ -59,7 +59,7 @@ } 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 @@ 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 @@ } 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 d8393489b4c4 -r 3ea1751e3fe7 tools/blktap2/drivers/tapdisk-interface.h --- a/tools/blktap2/drivers/tapdisk-interface.h Mon Jun 07 14:53:15 2010 -0700 +++ b/tools/blktap2/drivers/tapdisk-interface.h Mon Jun 07 14:53:15 2010 -0700 @@ -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 d8393489b4c4 -r 3ea1751e3fe7 tools/blktap2/drivers/tapdisk-vbd.c --- a/tools/blktap2/drivers/tapdisk-vbd.c Mon Jun 07 14:53:15 2010 -0700 +++ b/tools/blktap2/drivers/tapdisk-vbd.c Mon Jun 07 14:53:15 2010 -0700 @@ -82,6 +82,17 @@ 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 @@ 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 @@ 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 @@ 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 @@ err = tapdisk_vbd_add_block_cache(vbd); if (err) goto fail; - } + } err = tapdisk_vbd_validate_chain(vbd); if (err) @@ -432,16 +425,7 @@ 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 @@ char *params, *driver_str; td_vbd_driver_info_t *driver; - /* make a copy of path */ - /* TODO: check against MAX_NAME_LEM ? */ err = tapdisk_namedup(¶ms, 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); - list_del(&driver->next); - free(driver); - } +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); + } +} + /* 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 @@ 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 d8393489b4c4 -r 3ea1751e3fe7 tools/blktap2/drivers/tapdisk-vbd.h --- a/tools/blktap2/drivers/tapdisk-vbd.h Mon Jun 07 14:53:15 2010 -0700 +++ b/tools/blktap2/drivers/tapdisk-vbd.h Mon Jun 07 14:53:15 2010 -0700 @@ -80,7 +80,7 @@ }; 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 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 d8393489b4c4 -r 3ea1751e3fe7 tools/blktap2/include/list.h --- a/tools/blktap2/include/list.h Mon Jun 07 14:53:15 2010 -0700 +++ b/tools/blktap2/include/list.h Mon Jun 07 14:53:15 2010 -0700 @@ -87,24 +87,25 @@ 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) \