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, §ors, 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
