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

[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.

Paolo