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