From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41145) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uwrau-0008VD-UI for qemu-devel@nongnu.org; Wed, 10 Jul 2013 06:26:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uwrao-0001ti-A3 for qemu-devel@nongnu.org; Wed, 10 Jul 2013 06:25:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:27076) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uwran-0001tQ-Rc for qemu-devel@nongnu.org; Wed, 10 Jul 2013 06:25:50 -0400 Date: Wed, 10 Jul 2013 18:25:44 +0800 From: Fam Zheng Message-ID: <20130710102544.GA18465@T430s.nay.redhat.com> References: <1373268366-14508-1-git-send-email-cngesaint@gmail.com> <1373268366-14508-2-git-send-email-cngesaint@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1373268366-14508-2-git-send-email-cngesaint@gmail.com> Subject: Re: [Qemu-devel] [PATCH V2 1/5] Refine and export infinite loop checking in collect_image_info_list() Reply-To: famz@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Xu Wang Cc: kwolf@redhat.com, stefanha@gmail.com, qemu-devel@nongnu.org, xiawenc@linux.vnet.ibm.com On Mon, 07/08 03:26, Xu Wang wrote: > Signed-off-by: Xu Wang > --- > block.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++ > include/block/block.h | 4 ++ > qemu-img.c | 30 +++++---------- > 3 files changed, 114 insertions(+), 21 deletions(-) > > diff --git a/block.c b/block.c > index b88ad2f..53b1a01 100644 > --- a/block.c > +++ b/block.c > @@ -4431,6 +4431,107 @@ 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; > +} > + > +/** > + * Check backing file chain if there is a loop in it and build list of > + * ImageInfo if needed. > + * > + * @filename: topmost image filename, absolute or relative path is OK. > + * @fmt: topmost image format (may be NULL to autodetect) > + * @head: head of ImageInfo list. If not need please set head to null. This parameter is removed, you can also remove the comment. > + * @chain: true - enumerate entire backing file chain > + * false - only topmost image file Does it make sense when chain==false for loop check? > + * @backing_file: if this value is set, @filename inode (-1 if it doesn't > + * exist) will insert into hash table directly. Loop check > + * starts from it. Absolute or relative path is OK for it. > + * @backing_format: format of backing file Why these two parameters? The caller can still pass bs->backing_file to filename and bs->backing_fmt to fmt without them, to skip the topmost. > + * > + * If return true, stands for a backing file loop exists or some error > + * happened. If return false, everything is ok. Please format like this: + * Returns: true for backing file loop, false for no loop. > + */ > +bool bdrv_backing_file_loop_check(const char *filename, const char *fmt, > + bool chain, const char *backing_file, > + const char *backing_format) { > + GHashTable *inodes; > + BlockDriverState *bs; > + BlockDriver *drv; > + struct stat sbuf; > + long inode = 0; > + int ret; > + char fbuf[1024]; > + > + inodes = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL); > + > + if (backing_file) { > + /* Check if file exists. */ > + if (access(filename, F_OK)) { > + inode = -1; > + } else { > + if (stat(filename, &sbuf) == -1) { > + error_report("Get file %s stat failed.", filename); > + goto err; > + } > + inode = (long)sbuf.st_ino; > + } > + > + filename = backing_file; > + fmt = backing_format; > + g_hash_table_insert(inodes, (gpointer)&inode, NULL); > + } > + > + while (filename && (filename[0] != '\0')) { > + if (stat(filename, &sbuf) == -1) { Does it mean stat() on backing_file twice if it's not NULL? As you assigned backing_file to filename above. > + error_report("Get file %s stat failed.", filename); > + goto err; > + } > + inode = (long)sbuf.st_ino; > + > + if (g_hash_table_lookup_extended(inodes, &inode, NULL, NULL)) { > + error_report("Backing file '%s' creates an infinite loop.", > + filename); > + goto err; > + } > + g_hash_table_insert(inodes, (gpointer)&inode, NULL); > + > + bs = bdrv_new("image"); > + > + if (fmt) { > + drv = bdrv_find_format(fmt); > + if (!drv) { > + error_report("Unknown file format '%s'", fmt); > + goto err; bs is leaked. > + } > + } else { > + drv = NULL; > + } > + > + ret = bdrv_open(bs, filename, NULL, > + BDRV_O_CACHE_WB | BDRV_O_NO_BACKING, drv); > + if (ret < 0) { bs is leaked. > + error_report("Could not open '%s': %s", filename, strerror(-ret)); > + goto err; > + } These lines looks like a copy & paste from bdrv_new_open(). Could you just call it? > + > + filename = fmt = NULL; > + if (chain) { > + bdrv_get_backing_filename(bs, fbuf, sizeof(fbuf)); > + filename = fbuf; > + } > + > + bdrv_delete(bs); > + } > + g_hash_table_destroy(inodes); > + return false; > + > +err: > + g_hash_table_destroy(inodes); > + return true; > +} > + > 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 2307f67..6c631f2 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -332,6 +332,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_file_loop_check(const char *filename, const char *fmt, > + bool chain, const char *backing_file, > + const char *backing_format); Please align to parameter beginning: bool chain, const char *backing_file, const char *backing_format); > + > 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 809b4f1..48284c5 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1620,11 +1620,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 > * > @@ -1642,24 +1637,18 @@ 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; > - > - filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL); > + Please avoid trailing whitespaces. You could use scripts/checkpatch.pl to check your patch for coding style problems. > + /* check if loop exists and build ImageInfoList */ > + if (bdrv_backing_file_loop_check(filename, fmt, chain, NULL, NULL)) { > + goto err; > + } > > 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); > - > bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING, > false, false); > if (!bs) { > @@ -1692,12 +1681,11 @@ static ImageInfoList *collect_image_info_list(const char *filename, > } > } > } > - g_hash_table_destroy(filenames); > + > return head; > > err: > qapi_free_ImageInfoList(head); > - g_hash_table_destroy(filenames); > return NULL; > } > > -- > 1.8.1.4 > > -- Fam