From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52273) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VgTx7-0005CQ-QA for qemu-devel@nongnu.org; Wed, 13 Nov 2013 01:29:31 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VgTx0-0007Cl-Qc for qemu-devel@nongnu.org; Wed, 13 Nov 2013 01:29:25 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60811) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VgTx0-000764-Hk for qemu-devel@nongnu.org; Wed, 13 Nov 2013 01:29:18 -0500 Message-ID: <52831C2F.2030903@redhat.com> Date: Wed, 13 Nov 2013 14:29:03 +0800 From: Fam Zheng MIME-Version: 1.0 References: <1384310380-9805-1-git-send-email-gesaint@linux.vnet.ibm.com> <1384310380-9805-2-git-send-email-gesaint@linux.vnet.ibm.com> In-Reply-To: <1384310380-9805-2-git-send-email-gesaint@linux.vnet.ibm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH V7 1/4] block/qemu-img: Refine and export infinite loop checking in collect_image_info_list() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Xu Wang , qemu-devel@nongnu.org Cc: kwolf@redhat.com, wdongxu@linux.vnet.ibm.com, stefanha@gmail.com, Xu Wang , xiawenc@linux.vnet.ibm.com On 2013=E5=B9=B411=E6=9C=8813=E6=97=A5 10:39, 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 > --- > block.c | 76 ++++++++++++++++++++++++++++++++++++++++++= +++++++++ > include/block/block.h | 3 ++ > qemu-img.c | 52 +++++++++++++++++------------------ > 3 files changed, 105 insertions(+), 26 deletions(-) > > diff --git a/block.c b/block.c > index 58efb5b..3c43179 100644 > --- a/block.c > +++ b/block.c > @@ -4490,6 +4490,82 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCo= okie *cookie) > bs->total_time_ns[cookie->type] +=3D get_clock() - cookie->start_= time_ns; > } > > +static bool file_chain_has_loop(GHashTable *filenames, const char *fil= ename, > + const char *fmt) > +{ > + BlockDriverState *bs; > + char fbuf[PATH_MAX]; > + int ret; > + Error *local_err =3D NULL; > + > + while (filename && (filename[0] !=3D '\0')) { > + if (g_hash_table_lookup_extended(filenames, filename, NULL, NU= LL)) { > + error_report("Backing file '%s' creates an infinite loop."= , > + filename); > + return true; > + } I would expect the caller to format error message and report to user.=20 But this is acceptable. Let's see if other reviewers have more opinions. Fam > + g_hash_table_insert(filenames, (gpointer)filename, NULL); > + > + bs =3D bdrv_new("image"); > + > + ret =3D bdrv_open(bs, filename, NULL, > + BDRV_O_CACHE_WB | BDRV_O_NO_BACKING, NULL, &lo= cal_err); > + if (ret < 0) { > + error_report("Could not open '%s': %s", filename, > + error_get_pretty(local_err)); > + error_free(local_err); > + local_err =3D NULL; > + return true; > + } > + > + bdrv_get_backing_filename(bs, fbuf, sizeof(fbuf)); > + filename =3D fbuf; > + fmt =3D 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 aut= odetect). > + * @new_filename: if a new image to be created and takes @filename as = backing > + * file, the new chain should be checked before creatin= g. > + * > + * Returns: true for backing chain okay, false for loop happened. > + */ > +bool bdrv_backing_chain_okay(const char *filename, const char *fmt, > + const char *new_filename) > +{ > + GHashTable *filenames; > + > + filenames =3D g_hash_table_new_full(g_str_hash, g_str_equal, NULL,= NULL); > + > + if (filename =3D=3D NULL || filename[0] =3D=3D '\0') { > + goto exit; > + } > + > + if (new_filename && new_filename[0] !=3D '\0') { > + g_hash_table_insert(filenames, (gpointer)new_filename, NULL); > + } > + > + if (file_chain_has_loop(filenames, filename, fmt)) { > + 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..7ad714f 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -378,6 +378,9 @@ int bdrv_save_vmstate(BlockDriverState *bs, const u= int8_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, > + const char *new_filename); > + > 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..f8644c6 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -281,6 +281,14 @@ static BlockDriverState *bdrv_new_open(const char = *filename, > drv =3D NULL; > } > > + /* check backing file loop if the whole chain need to be opened */ > + if (!(flags & BDRV_O_NO_BACKING) && > + !bdrv_backing_chain_okay(filename, fmt, NULL)) { > + error_report("bdrv_new_open: Open %s failed. An infinite loop = exists " > + "in the backing chain", filename); > + goto fail; > + } > + > ret =3D 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(ImageInfo= List *list) > } > } > > -static gboolean str_equal_func(gconstpointer a, gconstpointer b) > -{ > - return strcmp(a, b) =3D=3D 0; > -} > - > /** > * Open an image file chain and return an ImageInfoList > * > @@ -1663,30 +1666,24 @@ static ImageInfoList *collect_image_info_list(c= onst char *filename, > bool chain) > { > ImageInfoList *head =3D NULL; > + BlockDriverState *bs; > + ImageInfoList *elem; > ImageInfoList **last =3D &head; > - GHashTable *filenames; > + ImageInfo *info; > Error *err =3D NULL; > + int flags =3D BDRV_O_FLAGS; > > - filenames =3D g_hash_table_new_full(g_str_hash, str_equal_func, NU= LL, NULL); > - > - while (filename) { > - BlockDriverState *bs; > - ImageInfo *info; > - ImageInfoList *elem; > - > - if (g_hash_table_lookup_extended(filenames, filename, NULL, NU= LL)) { > - error_report("Backing file '%s' creates an infinite loop."= , > - filename); > - goto err; > - } > - g_hash_table_insert(filenames, (gpointer)filename, NULL); > + if (!chain) { > + flags |=3D BDRV_O_NO_BACKING; > + } > > - bs =3D bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_B= ACKING, > - false, false); > - if (!bs) { > - goto err; > - } > + bs =3D 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(c= onst char *filename, > if (info->has_backing_filename_format) { > fmt =3D info->backing_filename_format; > } > + > + if (filename) { > + bs =3D 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; > } > >