On Tue, 11/05 22:09, 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 <gesa...@linux.vnet.ibm.com>
> ---
> block.c | 117
> ++++++++++++++++++++++++++++++++++++++++++++++++++
> include/block/block.h | 4 ++
> qemu-img.c | 44 ++++++++-----------
> 3 files changed, 139 insertions(+), 26 deletions(-)
>
> diff --git a/block.c b/block.c
> index 58efb5b..3443117 100644
> --- a/block.c
> +++ b/block.c
> @@ -4490,6 +4490,123 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie
> *cookie)
> bs->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns;
> }
>
> +static gboolean str_equal_func(gconstpointer a, gconstpointer b)
> +{
> + return strcmp(a, b) == 0;
> +}
Just use g_str_equal here.
> +
> +static bool file_chain_loop_check(GHashTable *filenames, const char
> *filename,
> + const char *fmt) {
Open brace '{' should be in a new line for functions.
Still confusing function name and return type. Suggest file_chain_has_loop().
> + BlockDriverState *bs;
> + BlockDriver *drv;
> + char fbuf[1024];
Could use PATH_MAX.
> + int ret;
> + Error *local_err = NULL;
> +
> + while (filename && (filename[0] != '\0')) {
> + if (g_hash_table_lookup_extended(filenames, filename, NULL, NULL)) {
> + error_report("Backing file '%s' creates an infinite loop.",
> + filename);
> + return true;
> + }
> + g_hash_table_insert(filenames, (gpointer)filename, NULL);
> +
> + bs = bdrv_new("image");
> +
> + if (fmt) {
> + drv = bdrv_find_format(fmt);
> + if (!drv) {
> + error_report("Unknown file format '%s'", fmt);
> + bdrv_delete(bs);
> + return true;
> + }
> + } else {
> + drv = NULL;
> + }
No need to call bdrv_find_format for multiple times. Also it doesn't look good
to write format checking here, just let the caller pass in drv.
> +
> + ret = bdrv_open(bs, filename, NULL,
> + BDRV_O_CACHE_WB | BDRV_O_NO_BACKING, drv,
> &local_err);
> + if (ret < 0) {
> + error_report("Could not open '%s': %s", filename,
> + error_get_pretty(local_err));
> + error_free(local_err);
> + local_err = NULL;
> + return true;
> + }
> +
> + bdrv_get_backing_filename(bs, fbuf, sizeof(fbuf));
> + filename = fbuf;
> + fmt = 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.
> + * @fmt: topmost image format of backing file chain(may be NULL to
> autodetect).
> + *
> + * Returns: true for backing file loop or error happened, false for no loop.
Really?
> + */
> +bool bdrv_backing_chain_okay(const char *filename, const char *fmt) {
> + GHashTable *filenames;
> +
> + if (filename == NULL || filename[0] == '\0') {
> + return true;
Please don't mix "goto err" and multiple "return true". Could be
goto exit;
...
> + }
> +
> + filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL,
> NULL);
> +
> + if (file_chain_loop_check(filenames, filename, fmt)) {
> + goto err;
> + }
> +
> + g_hash_table_destroy(filenames);
exit:
> + return true;
> +
> +err:
> + g_hash_table_destroy(filenames);
> + return false;
> +}
> +
> +/**
> + * Check if there is loop exists in the backing file chain and if there will
> + * be loop occur after backing file chain updated or new image created.
> + *
> + * @filename: the image filename to be created.
> + * @backing_file: topmost image filename of backing file chain.
> + * @backing_fmt: topmost image format (may be NULL to autodetect).
> + *
> + * Returns: true for backing file loop or error happened, false for no loop.
I don't think so.
> + */
> +bool bdrv_new_chain_okay(const char *filename, const char *backing_file,
> + const char *backing_fmt) {
Please align arguments: ^
This function could be merged to bdrv_backing_chain_ok by adding an optional
argument const char *new_filename. If it's not NULL you add it to the hash
table, like:
bool bdrv_backing_chain_okay(const char *filename, const char *fmt,
const char *new_filename)
{
GHashTable *filenames;
if (filename == NULL || filename[0] == '\0') {
goto exit;
}
filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL,
NULL);
if (new_filename && new_filename[0] != '\0') {
g_hash_table_insert(filenames, (gpointer)filename, NULL);
}
if (file_chain_loop_check(filenames, filename, fmt)) {
goto err;
}
g_hash_table_destroy(filenames);
exit:
return true;
err:
g_hash_table_destroy(filenames);
return false;
}
> + GHashTable *filenames;
> +
> + if (backing_file == NULL || backing_file[0] == '\0') {
> + return true;
> + }
> +
> + filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL,
> NULL);
> +
> + g_hash_table_insert(filenames, (gpointer)filename, NULL);
> +
> + if (file_chain_loop_check(filenames, backing_file, backing_fmt)) {
> + goto err;
> + }
> +
> + 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..0945c09 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -378,6 +378,10 @@ 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, const char *fmt);
> +bool bdrv_new_chain_okay(const char *filename, const char *backing_file,
> + const char *backing_fmt);
Please align arguments: ^
> +
> 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 bf3fb4f..d5ec45b 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1641,11 +1641,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 +1658,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 +1700,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;
> }
Is backing chain loop still checked in collect_image_info_list()?
Fam
