Re: [Qemu-devel] [PATCH V5 1/6] block/qemu-img: Refine and export infinite loop checking in collect_image_info_list()

[Xu Wang <gesaint@linux.vnet.ibm.com>]

于 2013/8/3 6:09, Eric Blake 写道:
> On 08/02/2013 03:02 AM, Xu Wang wrote:
>> From: Xu Wang<cngesaint@gmail.com>
>>
>> If there is a loop exists in the backing file chain, many problems
>> could be caused by it, such as no response and segment fault during
>> system boot. Hence stopping backing file loop appear is very necessary.
>> This patch refine and export loop checking function from collect_image_
>> info_list() to block.c and build a independent function named bdrv_
>> backing_file_loop_check().
> Are we trying to get this series into 1.6 on the grounds that it fixes
> bugs?  The grammar was a bit awkward; may I suggest:
>
> 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 an independent function
> bdrv_backing_file_loop_check().
I am very sorry for working other works so long time and do not submit new
version patches in time. Now I am rewriting them to improve their quality.
>> Signed-off-by: Xu Wang<cngesaint@gmail.com>
>> ---
>>   block.c               | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/block/block.h |  4 +++
>>   qemu-img.c            | 44 ++++++++++-------------
>>   3 files changed, 118 insertions(+), 26 deletions(-)
>>
>>   
>> +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.
>> + *
>> + * @filename: topmost image filename, absolute or relative path is OK.
>> + * @fmt: topmost image format (may be NULL to autodetect)
>> + * @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.
> This is confusing - are we passing in a filename or an inode?  At the
> parameter level, it doesn't matter what we hash or even that we have a
> hash table - just what we pass in.  The choice of hashing an inode value
> is an internal detail.
In the new version I'll just pass filename and compare them to find loop.
>> + * @backing_format: format of backing file
> Why do we need to pass in two filenames?  I'm guessing it's because you
> want to detect TWO types of loops:
>
> Loop 1 - we are creating a NEW file, but requesting a backing file name
> - if anything in that backing file chain points to the file we are about
> to create, we would be creating a loop, and want to fail.  That is, if
> we have an existing file "B" that points to a missing backing file "A",
> and we are now creating file "A" with a backing file of "B", we want the
> creation to fail because it would create a loop.  But in this case, it
> is sufficient to note that "B" is a broken image (since "A" doesn't
> exist yet), so we don't have to do a loop check here, rather we can just
> merely test if "B" and its entire backing chain is reachable.
>
> Loop 2 - we are testing if an EXISTING file has a loop.  But the loop
> may not necessarily point back to our own file.  That is, a file "A"
> with metadata that claims a backing file of "A" is a loop.  Also, a file
> "A" with metadata that claims "B" as backing, and existing "B" with
> metadata that claims "A" as backing, is a loop.  But we ALSO want to
> detect and reject a file "A" that claims "B" as backing, where an
> existing file "B" claims itself as backing; or even "A" claims "B", "B"
> claims "C", and "C" claims "B".  Thus, we need to detect the loop no
> matter where in the chain it occurs, and realize that it does not
> necessarily point back to "A".  So, why not just declare that we are
> starting with "A", without regards to it's backing file?
>
> That is, I think your function signature is over-engineered - you really
> only need to pass in a single file name - that of an existing file, and
> have the file report success if all backing files can be resolved
> without hitting any loops in the resolution, and failure if any backing
> file is missing or if any backing file refers back to a point earlier in
> the chain.
If we execute 'qemu-img create ...' command to create a new image and point
set a backing file. I should check if there is loop exists in the 
backing file
chain (as you said above). But this function also need to verify the new
image to be created won't overwrite any one in the backing file chain. 
Because
that will create a loop. If I check the whole chain after the new image 
created,
it's too late to find the loop (if it exists) because some image has 
been overwrited.
So is there any suggestion if I just pass one filename to the function? 
Thanks.
>> + *
>> + * Returns: true for backing file loop or error happened, false for no loop.
>> + */
>> +bool bdrv_backing_file_loop_check(const char *filename, const char *fmt,
> Those return semantics are a bit unusual.  A function named _check()
> that returns a bool usually returns true when the function passed.  How
> will this function usually be called?  Let's name it something that will
> avoid double-negatives; maybe:
>
> bool bdrv_backing_chain_okay(const char *filename, const char *fmt)
>
> which returns true if filename is safe to use as a backing chain.
Good name. I'll change name of this function into it:-)
>
>> +                                  const char *backing_file,
>> +                                  const char *backing_format) {
>> +    GHashTable *inodes;
>> +    BlockDriverState *bs;
>> +    BlockDriver *drv;
>> +    struct stat sbuf;
>> +    long inode = 0;
> You are not guaranteed that st_ino fits in a long; use ino_t.  (IIRC, on
> 32-bit Cygwin, ino_t is 64-bit, but long is 32-bit)
Sure. I'll update all points like this.
>> +    int ret;
>> +    char fbuf[1024];
>> +
>> +    inodes = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
> So this says you are comparing by name (you are doing strcmp on the
> hashed data)...
Comparing by name is good choice. I'll update it in my new version.
>> +
>> +    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);
> ...but here, you are shoving in integers cast as a name.  strcmp() on
> integers will not work.  Furthermore, inode alone is not guaranteed to
> be unique; you are only guaranteed that the combination of dev_t and
> ino_t form a unique identifier for any file (that is, you are prone to
> false collisions if two files on different devices happen to have the
> same inode).
> I want to see loop detection working, but this patch still needs an
> overhaul before it can be considered working.
Thank you very much for your suggestions:-)
   Xu Wang