Re: [Qemu-devel] [PATCH 21/47] block: add bdrv_ensure_backing_file

[Kevin Wolf <kwolf@redhat.com>]

Am 11.09.2012 15:46, schrieb Paolo Bonzini:
> 
>> Once again, combining code motion and code changes in one patch makes
>> it harder to review.
> 
> bdrv_ensure_backing_file() is a new standalone function that happens to be
> usable in bdrv_open as well.  But I can separate the changes/fixes to a
> separate patch.
> 
> In particular it is can be used after a file has been opened with
> BDRV_O_NO_BACKING (at which point the flag does not reflect reality
> anymore, hence the removal of the flag).

Yes, that's what I figured eventually. Maybe some documentation for the
function couldn't hurt.

>> bdrv_ensure_backing_file() isn't a good name, after reading only the
>> subject line I had no idea what this function might do. It's still
>> not entirely clear to me what the different to a bdrv_open_backing_file()
>> is, except that it doesn't do anything if a backing file is already
>> open. In which cases do we rely on this behaviour?
> 
> We open the mirroring target with BDRV_O_NO_BACKING usually, but require
> the backing file if the cluster size is larger than the dirty block
> granularity.  Later, COW is done in the mirror job, so this is not
> needed anymore at the end of the series.

Can we then put a /* FIXME */ comment there and revert that behaviour at
the end of the series? Then we can call it bdrv_open_backing_file() and
it's meaning becomes more obvious.

>>> +    if (bs->backing_file[0] == '\0') {
>>> +        return 0;
>>> +    }
>>> +
>>> +    bs->backing_hd = bdrv_new("");
>>> +    bdrv_get_full_backing_filename(bs, backing_filename,
>>> +                                   sizeof(backing_filename));
>>> +
>>> +    if (bs->backing_format[0] != '\0') {
>>> +        back_drv = bdrv_find_format(bs->backing_format);
>>> +    }
>>> +
>>> +    /* backing files always opened read-only */
>>> +    back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT);
>>> +
>>> +    ret = bdrv_open(bs->backing_hd, backing_filename, back_flags,
>>> back_drv);
>>> +    if (ret < 0) {
>>> +        bdrv_close(bs);
>>
>> I don't like this because it makes the invalid assumption that the
>> caller has just opened bs and wants to close it if opening the
>> backing file fails. I think this is part of the error handling that belongs
>> in the caller: It opened bs, so it is responsible for closing it in
>> error cases.
> 
> It's a bug, it should have closed bs->backing_hd.

Are you sure? You removed the bdrv_close(bs) in the caller, so that it's
missing there would be a second bug.

An explicit bdrv_close(bs->backing_hd) isn't required here, it is
implicitly called in bdrv_delete(bs->backing_hd).

>>> +        bdrv_delete(bs->backing_hd);
>>
>> This is a bug fix of its own, should be a separate patch.
> 
> Ok.
> 
>>> +        bs->backing_hd = NULL;
>>> +        return ret;
>>> +    }
>>> +    if (bs->is_temporary) {
>>> +        bs->backing_hd->keep_read_only = !(bs->open_flags &
>>> BDRV_O_RDWR);
>>> +    } else {
>>> +        /* base images use the same setting as leaf */
>>
>> Huh, "parent" turned into "leaf"?
> 
> Will move this to a separate patch, too.

I don't even understand what you're trying to say in this comment. The
only tree that I can think of (so something like leaves exists) is built
by bs->file and bs->backing_hd. But in this case, the top-level image,
from which the flags are taken, is the root and not a leaf?

Kevin