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
