Re: [Qemu-devel] [PATCH 1/2] Refine and export infinite loop checking in collect_image_info_list()

[Eric Blake]

On 06/27/2013 01:38 AM, Xu Wang wrote:
> From: Xu Wang <address@hidden>
> 
> Signed-off-by: Xu Wang <address@hidden>
> ---
>  qemu-img.c | 110 
> +++++++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 89 insertions(+), 21 deletions(-)
> 

> +/**
> + * Check backing file chain if there is a loop in it and build list of
> + * ImageInfo if needed.
> + *
> + * @filename: topmost image filename

absolute? relative?

> + * @fmt: topmost image format (may be NULL to autodetect)
> + * @head: head of ImageInfo list. If not need please set head to null.
> + * @chain: true  - enumerate entire backing file chain
> + *         false - only topmost image file
> + * @backing_file: if this value is set, filename will insert into hash
> + *                table directly. open and seek backing file start from it.
> + *
> + * If return true, stands for a backing file loop exists or some error
> + * happend. If return false, everything is ok.

s/happend/happened/

> + */
> +static bool backing_file_loop_check(const char *filename, const char *fmt,
> +                             bool chain, const char *backing_file) {

Indentation is off.

> +    GHashTable *filenames;
> +    BlockDriverState *bs;
> +    ImageInfo *info;
> +    Error *err = NULL;
> +
> +    filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, 
> NULL);
> +
> +    /* If backing file exists, filename will insert into hash table and seek
> +     * the whole backing file chain from @backing_file.
> +     */
> +    if (backing_file) {
> +        g_hash_table_insert(filenames, (gpointer)filename, NULL);

Does this have any false positives (perhaps mishandling due to relative
names) or false negatives (perhaps hard links allow different spellings
of the same file to create a loop, although the difference in names
won't indicate the problem)?  I'd really like to see you add a testcase
before this patch gets committed, although I agree that a patch along
these lines is worthwhile.  For example, make sure the following chain
is not rejected:

/dir1/base.img <- /dir1/wrap.img(relative backing 'base.img') <-
/dir2/base.img (absolute backing '/dir1/base.img') <-
/dir2/wrap.img(relative backing 'base.img')

whether opened in /dir2/ via relative name 'wrap.img' or absolute name
'/dir2/wrap.img'.  Likewise, make sure you can detect this loop:

create directory 'dir'
create './dir/b.img'
create './b.img' with relative backing 'dir/b.img'
remove ./dir/b.img and dir
ln -s . dir
now 'b.img' refers to itself as backing file, even though the names
./b.img and ./dir/b.img are not equal by strcmp.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature