Message ID | 1385447913-19004-2-git-send-email-gesaint@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
On Tue, Nov 26, 2013 at 01:38:30AM -0500, Xu Wang wrote: > If there is a loop in the backing file chain, it could cause problems > such as no response or a segfault during system boot. Hence detecting a > backing file loop is necessary. This patch extracts the loop check from > collect_image_info_list() in block.c into independent functions > bdrv_backing_chain_okay() and bdrv_image_create_okay(). > > Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> > --- > block.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++ > include/block/block.h | 3 +++ > qemu-img.c | 52 ++++++++++++++++++------------------ > 3 files changed, 102 insertions(+), 26 deletions(-) > > diff --git a/block.c b/block.c > index 382ea71..7016ce8 100644 > --- a/block.c > +++ b/block.c > @@ -4497,6 +4497,79 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie) > bs->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns; > } > > +static bool file_chain_has_loop(GHashTable *filenames, const char *filename, > + BlockDriver *drv, Error **errp) > +{ > + BlockDriverState *bs; > + char fbuf[PATH_MAX]; > + Error *local_err = NULL; > + int ret; > + > + while (filename && (filename[0] != '\0')) { > + if (g_hash_table_lookup_extended(filenames, filename, NULL, NULL)) { > + error_setg(errp, "Backing file '%s' creates an infinite loop.", > + filename); > + return true; > + } > + g_hash_table_insert(filenames, (gpointer)filename, NULL); > + > + bs = bdrv_new("image"); > + ret = bdrv_open(bs, filename, NULL, > + BDRV_O_CACHE_WB | BDRV_O_NO_BACKING, drv, &local_err); > + if (ret < 0) { > + error_setg(errp, "Could not open '%s': %s", filename, > + error_get_pretty(local_err)); > + return true; This leaks *bs at this point. > + } > + > + bdrv_get_backing_filename(bs, fbuf, sizeof(fbuf)); > + filename = fbuf; > + drv = NULL; > + > + bdrv_unref(bs); > + } > + > + return false; > +} > + > +/** > + * Check backing file chain if there is a loop in it. > + * > + * @filename: topmost image filename of backing file chain. > + * @drv: topmost image driver(may be NULL to autodetect). > + * @new_filename: if a new image to be created and takes @filename as backing > + * file, the new chain should be checked before creating. > + * > + * Returns: true for backing chain okay, false for loop happened. > + */ > +bool bdrv_backing_chain_okay(const char *filename, BlockDriver *drv, > + const char *new_filename, Error **errp) > +{ > + GHashTable *filenames; > + > + filenames = g_hash_table_new_full(g_str_hash, g_str_equal, NULL, NULL); > + > + if (filename == NULL || filename[0] == '\0') { > + goto exit; > + } > + > + if (new_filename && new_filename[0] != '\0') { > + g_hash_table_insert(filenames, (gpointer)new_filename, NULL); > + } > + > + if (file_chain_has_loop(filenames, filename, drv, errp)) { > + goto err; > + } > + > +exit: > + g_hash_table_destroy(filenames); > + return true; > + > +err: > + g_hash_table_destroy(filenames); > + return false; > +} > + Minor nit, but it would be nice to have a single cleanup path. E.g., something like this: bool ret; ... exit: g_hash_table_destroy(filenames); return ret; Then just set ret as appropriate in each 'if' statement in the function, and jump to 'exit'. > void bdrv_img_create(const char *filename, const char *fmt, > const char *base_filename, const char *base_fmt, > char *options, uint64_t img_size, int flags, > diff --git a/include/block/block.h b/include/block/block.h > index 3560deb..f5e84dc 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -378,6 +378,9 @@ int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf, > int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf, > int64_t pos, int size); > > +bool bdrv_backing_chain_okay(const char *filename, BlockDriver *drv, > + const char *new_filename, Error **errp); > + > void bdrv_img_create(const char *filename, const char *fmt, > const char *base_filename, const char *base_fmt, > char *options, uint64_t img_size, int flags, > diff --git a/qemu-img.c b/qemu-img.c > index b6b5644..1f38267 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -281,6 +281,14 @@ static BlockDriverState *bdrv_new_open(const char *filename, > drv = NULL; > } > > + /* check backing file loop if the whole chain need to be opened */ > + if (!(flags & BDRV_O_NO_BACKING) && > + !bdrv_backing_chain_okay(filename, drv, NULL, &local_err)) { > + error_report("bdrv_new_open: Open %s failed: %s", filename, > + error_get_pretty(local_err)); > + goto fail; > + } > + > ret = bdrv_open(bs, filename, NULL, flags, drv, &local_err); > if (ret < 0) { > error_report("Could not open '%s': %s", filename, > @@ -1641,11 +1649,6 @@ static void dump_human_image_info_list(ImageInfoList *list) > } > } > > -static gboolean str_equal_func(gconstpointer a, gconstpointer b) > -{ > - return strcmp(a, b) == 0; > -} > - > /** > * Open an image file chain and return an ImageInfoList > * > @@ -1663,30 +1666,24 @@ static ImageInfoList *collect_image_info_list(const char *filename, > bool chain) > { > ImageInfoList *head = NULL; > + BlockDriverState *bs; > + ImageInfoList *elem; > ImageInfoList **last = &head; > - GHashTable *filenames; > + ImageInfo *info; > Error *err = NULL; > + int flags = BDRV_O_FLAGS; > > - filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL); > - > - while (filename) { > - BlockDriverState *bs; > - ImageInfo *info; > - ImageInfoList *elem; > - > - if (g_hash_table_lookup_extended(filenames, filename, NULL, NULL)) { > - error_report("Backing file '%s' creates an infinite loop.", > - filename); > - goto err; > - } > - g_hash_table_insert(filenames, (gpointer)filename, NULL); > + if (!chain) { > + flags |= BDRV_O_NO_BACKING; > + } > > - bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING, > - false, false); > - if (!bs) { > - goto err; > - } > + bs = bdrv_new_open(filename, fmt, flags, > + false, false); > + if (!bs) { > + goto err; > + } > > + while (filename) { > bdrv_query_image_info(bs, &info, &err); > if (error_is_set(&err)) { > error_report("%s", error_get_pretty(err)); Between here, and the next hunk of this patch, there is a bdrv_unref(bs) in the original code. This becomes important later: > @@ -1711,14 +1708,17 @@ static ImageInfoList *collect_image_info_list(const char *filename, > if (info->has_backing_filename_format) { > fmt = info->backing_filename_format; > } > + > + if (filename) { > + bs = bdrv_find_backing_image(bs, filename); > + } This function will now most likely blow up on qemu-img info --backing-chain, if the image file has any backing files. Edit: just checked; it does indeed segfault: # ~/deploy/bin/qemu-img info --backing-chain /tmp/snap3.qcow2 Segmentation fault (core dumped) This patch does not have the context to show it, however as mentioned above, the bdrv_unref(bs) is performed, yet bs is still used afterwards in this function. Once bdrv_unref() has been called by a function, the BDS that was unreferenced can no longer be used. > } > } > - g_hash_table_destroy(filenames); > + > return head; > > err: > qapi_free_ImageInfoList(head); > - g_hash_table_destroy(filenames); > return NULL; > } > > -- > 1.8.1.4 > >
On Tue, Nov 26, 2013 at 01:38:30AM -0500, Xu Wang wrote: > If there is a loop in the backing file chain, it could cause problems > such as no response or a segfault during system boot. Hence detecting a > backing file loop is necessary. This patch extracts the loop check from > collect_image_info_list() in block.c into independent functions > bdrv_backing_chain_okay() and bdrv_image_create_okay(). > > Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> > --- > block.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++ > include/block/block.h | 3 +++ > qemu-img.c | 52 ++++++++++++++++++------------------ > 3 files changed, 102 insertions(+), 26 deletions(-) > > diff --git a/block.c b/block.c > index 382ea71..7016ce8 100644 > --- a/block.c > +++ b/block.c > @@ -4497,6 +4497,79 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie) > bs->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns; > } > > +static bool file_chain_has_loop(GHashTable *filenames, const char *filename, > + BlockDriver *drv, Error **errp) > +{ > + BlockDriverState *bs; > + char fbuf[PATH_MAX]; > + Error *local_err = NULL; > + int ret; > + > + while (filename && (filename[0] != '\0')) { > + if (g_hash_table_lookup_extended(filenames, filename, NULL, NULL)) { > + error_setg(errp, "Backing file '%s' creates an infinite loop.", > + filename); > + return true; > + } > + g_hash_table_insert(filenames, (gpointer)filename, NULL); > + > + bs = bdrv_new("image"); > + ret = bdrv_open(bs, filename, NULL, > + BDRV_O_CACHE_WB | BDRV_O_NO_BACKING, drv, &local_err); > + if (ret < 0) { > + error_setg(errp, "Could not open '%s': %s", filename, > + error_get_pretty(local_err)); This also leaks local_err here - you should call error_free(local_err). > + return true; > + } > + > + bdrv_get_backing_filename(bs, fbuf, sizeof(fbuf)); > + filename = fbuf; > + drv = NULL; > + > + bdrv_unref(bs); > + } > + > + return false; > +} > + > +/** > + * Check backing file chain if there is a loop in it. > + * > + * @filename: topmost image filename of backing file chain. > + * @drv: topmost image driver(may be NULL to autodetect). > + * @new_filename: if a new image to be created and takes @filename as backing > + * file, the new chain should be checked before creating. > + * > + * Returns: true for backing chain okay, false for loop happened. > + */ > +bool bdrv_backing_chain_okay(const char *filename, BlockDriver *drv, > + const char *new_filename, Error **errp) > +{ > + GHashTable *filenames; > + > + filenames = g_hash_table_new_full(g_str_hash, g_str_equal, NULL, NULL); > + > + if (filename == NULL || filename[0] == '\0') { > + goto exit; > + } > + > + if (new_filename && new_filename[0] != '\0') { > + g_hash_table_insert(filenames, (gpointer)new_filename, NULL); > + } > + > + if (file_chain_has_loop(filenames, filename, drv, errp)) { > + goto err; > + } > + > +exit: > + g_hash_table_destroy(filenames); > + return true; > + > +err: > + g_hash_table_destroy(filenames); > + return false; > +} > + > void bdrv_img_create(const char *filename, const char *fmt, > const char *base_filename, const char *base_fmt, > char *options, uint64_t img_size, int flags, > diff --git a/include/block/block.h b/include/block/block.h > index 3560deb..f5e84dc 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -378,6 +378,9 @@ int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf, > int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf, > int64_t pos, int size); > > +bool bdrv_backing_chain_okay(const char *filename, BlockDriver *drv, > + const char *new_filename, Error **errp); > + > void bdrv_img_create(const char *filename, const char *fmt, > const char *base_filename, const char *base_fmt, > char *options, uint64_t img_size, int flags, > diff --git a/qemu-img.c b/qemu-img.c > index b6b5644..1f38267 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -281,6 +281,14 @@ static BlockDriverState *bdrv_new_open(const char *filename, > drv = NULL; > } > > + /* check backing file loop if the whole chain need to be opened */ > + if (!(flags & BDRV_O_NO_BACKING) && > + !bdrv_backing_chain_okay(filename, drv, NULL, &local_err)) { > + error_report("bdrv_new_open: Open %s failed: %s", filename, > + error_get_pretty(local_err)); > + goto fail; > + } > + > ret = bdrv_open(bs, filename, NULL, flags, drv, &local_err); > if (ret < 0) { > error_report("Could not open '%s': %s", filename, > @@ -1641,11 +1649,6 @@ static void dump_human_image_info_list(ImageInfoList *list) > } > } > > -static gboolean str_equal_func(gconstpointer a, gconstpointer b) > -{ > - return strcmp(a, b) == 0; > -} > - > /** > * Open an image file chain and return an ImageInfoList > * > @@ -1663,30 +1666,24 @@ static ImageInfoList *collect_image_info_list(const char *filename, > bool chain) > { > ImageInfoList *head = NULL; > + BlockDriverState *bs; > + ImageInfoList *elem; > ImageInfoList **last = &head; > - GHashTable *filenames; > + ImageInfo *info; > Error *err = NULL; > + int flags = BDRV_O_FLAGS; > > - filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL); > - > - while (filename) { > - BlockDriverState *bs; > - ImageInfo *info; > - ImageInfoList *elem; > - > - if (g_hash_table_lookup_extended(filenames, filename, NULL, NULL)) { > - error_report("Backing file '%s' creates an infinite loop.", > - filename); > - goto err; > - } > - g_hash_table_insert(filenames, (gpointer)filename, NULL); > + if (!chain) { > + flags |= BDRV_O_NO_BACKING; > + } > > - bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING, > - false, false); > - if (!bs) { > - goto err; > - } > + bs = bdrv_new_open(filename, fmt, flags, > + false, false); > + if (!bs) { > + goto err; > + } > > + while (filename) { > bdrv_query_image_info(bs, &info, &err); > if (error_is_set(&err)) { > error_report("%s", error_get_pretty(err)); > @@ -1711,14 +1708,17 @@ static ImageInfoList *collect_image_info_list(const char *filename, > if (info->has_backing_filename_format) { > fmt = info->backing_filename_format; > } > + > + if (filename) { > + bs = bdrv_find_backing_image(bs, filename); > + } > } > } > - g_hash_table_destroy(filenames); > + > return head; > > err: > qapi_free_ImageInfoList(head); > - g_hash_table_destroy(filenames); > return NULL; > } > > -- > 1.8.1.4 > >
diff --git a/block.c b/block.c index 382ea71..7016ce8 100644 --- a/block.c +++ b/block.c @@ -4497,6 +4497,79 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie) bs->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns; } +static bool file_chain_has_loop(GHashTable *filenames, const char *filename, + BlockDriver *drv, Error **errp) +{ + BlockDriverState *bs; + char fbuf[PATH_MAX]; + Error *local_err = NULL; + int ret; + + while (filename && (filename[0] != '\0')) { + if (g_hash_table_lookup_extended(filenames, filename, NULL, NULL)) { + error_setg(errp, "Backing file '%s' creates an infinite loop.", + filename); + return true; + } + g_hash_table_insert(filenames, (gpointer)filename, NULL); + + bs = bdrv_new("image"); + ret = bdrv_open(bs, filename, NULL, + BDRV_O_CACHE_WB | BDRV_O_NO_BACKING, drv, &local_err); + if (ret < 0) { + error_setg(errp, "Could not open '%s': %s", filename, + error_get_pretty(local_err)); + return true; + } + + bdrv_get_backing_filename(bs, fbuf, sizeof(fbuf)); + filename = fbuf; + drv = NULL; + + bdrv_unref(bs); + } + + return false; +} + +/** + * Check backing file chain if there is a loop in it. + * + * @filename: topmost image filename of backing file chain. + * @drv: topmost image driver(may be NULL to autodetect). + * @new_filename: if a new image to be created and takes @filename as backing + * file, the new chain should be checked before creating. + * + * Returns: true for backing chain okay, false for loop happened. + */ +bool bdrv_backing_chain_okay(const char *filename, BlockDriver *drv, + const char *new_filename, Error **errp) +{ + GHashTable *filenames; + + filenames = g_hash_table_new_full(g_str_hash, g_str_equal, NULL, NULL); + + if (filename == NULL || filename[0] == '\0') { + goto exit; + } + + if (new_filename && new_filename[0] != '\0') { + g_hash_table_insert(filenames, (gpointer)new_filename, NULL); + } + + if (file_chain_has_loop(filenames, filename, drv, errp)) { + goto err; + } + +exit: + g_hash_table_destroy(filenames); + return true; + +err: + g_hash_table_destroy(filenames); + return false; +} + void bdrv_img_create(const char *filename, const char *fmt, const char *base_filename, const char *base_fmt, char *options, uint64_t img_size, int flags, diff --git a/include/block/block.h b/include/block/block.h index 3560deb..f5e84dc 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -378,6 +378,9 @@ int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf, int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf, int64_t pos, int size); +bool bdrv_backing_chain_okay(const char *filename, BlockDriver *drv, + const char *new_filename, Error **errp); + void bdrv_img_create(const char *filename, const char *fmt, const char *base_filename, const char *base_fmt, char *options, uint64_t img_size, int flags, diff --git a/qemu-img.c b/qemu-img.c index b6b5644..1f38267 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -281,6 +281,14 @@ static BlockDriverState *bdrv_new_open(const char *filename, drv = NULL; } + /* check backing file loop if the whole chain need to be opened */ + if (!(flags & BDRV_O_NO_BACKING) && + !bdrv_backing_chain_okay(filename, drv, NULL, &local_err)) { + error_report("bdrv_new_open: Open %s failed: %s", filename, + error_get_pretty(local_err)); + goto fail; + } + ret = bdrv_open(bs, filename, NULL, flags, drv, &local_err); if (ret < 0) { error_report("Could not open '%s': %s", filename, @@ -1641,11 +1649,6 @@ static void dump_human_image_info_list(ImageInfoList *list) } } -static gboolean str_equal_func(gconstpointer a, gconstpointer b) -{ - return strcmp(a, b) == 0; -} - /** * Open an image file chain and return an ImageInfoList * @@ -1663,30 +1666,24 @@ static ImageInfoList *collect_image_info_list(const char *filename, bool chain) { ImageInfoList *head = NULL; + BlockDriverState *bs; + ImageInfoList *elem; ImageInfoList **last = &head; - GHashTable *filenames; + ImageInfo *info; Error *err = NULL; + int flags = BDRV_O_FLAGS; - filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL); - - while (filename) { - BlockDriverState *bs; - ImageInfo *info; - ImageInfoList *elem; - - if (g_hash_table_lookup_extended(filenames, filename, NULL, NULL)) { - error_report("Backing file '%s' creates an infinite loop.", - filename); - goto err; - } - g_hash_table_insert(filenames, (gpointer)filename, NULL); + if (!chain) { + flags |= BDRV_O_NO_BACKING; + } - bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING, - false, false); - if (!bs) { - goto err; - } + bs = bdrv_new_open(filename, fmt, flags, + false, false); + if (!bs) { + goto err; + } + while (filename) { bdrv_query_image_info(bs, &info, &err); if (error_is_set(&err)) { error_report("%s", error_get_pretty(err)); @@ -1711,14 +1708,17 @@ static ImageInfoList *collect_image_info_list(const char *filename, if (info->has_backing_filename_format) { fmt = info->backing_filename_format; } + + if (filename) { + bs = bdrv_find_backing_image(bs, filename); + } } } - g_hash_table_destroy(filenames); + return head; err: qapi_free_ImageInfoList(head); - g_hash_table_destroy(filenames); return NULL; }
If there is a loop in the backing file chain, it could cause problems such as no response or a segfault during system boot. Hence detecting a backing file loop is necessary. This patch extracts the loop check from collect_image_info_list() in block.c into independent functions bdrv_backing_chain_okay() and bdrv_image_create_okay(). Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- block.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/block/block.h | 3 +++ qemu-img.c | 52 ++++++++++++++++++------------------ 3 files changed, 102 insertions(+), 26 deletions(-)