Re: [Qemu-devel] [PATCH 3/8] vmdk: clean up open

[Kevin Wolf <kwolf@redhat.com>]

Am 20.10.2011 12:12, schrieb Paolo Bonzini:
> On 10/20/2011 11:28 AM, Kevin Wolf wrote:
>> This code is moved into bdrv_open, but there's another path how this
>> code can be reached:
>>
>> vmdk_parse_extents() ->  vmdk_open_sparse() ->  vmdk_open_vmdk4() ->
>> vmdk_open_desc_file().
>>
>> Don't we forget to open the parent file there now?
> 
> Let's look at the call chain:
> 
> vmdk_open:
>         return vmdk_open_desc_file(bs, flags, 0);
> 
> -> vmdk_open_desc_file:
>     ret = vmdk_parse_extents(buf, bs, bs->file->filename);
> 
> -> vmdk_parse_extents:
> 
> p points into vmdk_open_desc_file's buf:
> 
>         ret = sscanf(p, "%10s %" SCNd64 " %10s %511s %" SCNd64,
>                 access, &sectors, type, fname, &flat_offset);
>         path_combine(extent_path, sizeof(extent_path),
>                 desc_file_path, fname);
>         ret = bdrv_file_open(&extent_file, extent_path, bs->open_flags);
>             ret = vmdk_open_sparse(bs, extent_file, bs->open_flags);
> 
> So here it finds the sub-vmdk, opens it and passes it to vmdk_open_sparse.
> 
> -> vmdk_open_sparse:
>             return vmdk_open_vmdk4(bs, file, flags);
> 
> The extent_file is then passed to vmdk_open_vmdk4:
> 
> -> vmdk_open_vmdk4:
>     ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
>         return vmdk_open_desc_file(bs, flags, header.desc_offset << 9);
> 
> This reads the header from extent_file, but vmdk_open_desc_file will
> still read from bs->file.
> -> vmdk_open_desc_file:
>     s->desc_offset = 0;
>     if (vmdk_parent_open(bs)) {
> -> vmdk_parent_open:
>     if (bdrv_pread(bs->file, s->desc_offset, desc, DESC_SIZE) != DESC_SIZE) {
> 
> So s->desc_offset is always zero, and bs->file never changes.  So the data
> that vmdk_parent_open reads comes always from the same place, and anyway
> there have only one place where it can write it:
> 
> vmdk_parent_open:
>         pstrcpy(bs->backing_file, end_name - p_name + 1, p_name);
> 
> So the code is completely bogus in the case of recursive calls to
> vmdk_open_desc_file with parent images.  Maybe it simply cannot happen,
> and in that case the patched code is less bogus.
> 
> It is also possible that it can happen, and the bug is in setting
> s->desc_offset to zero.  Even in that case, calling vmdk_parent_open
> once would be correct and the patched code is less bogus.

Ok, if it can't happen it seems reasonable and could still qualify as
cleanup. Maybe you can add a line or two to the commit message for v2.

Kevin