Re: [Qemu-devel] [PATCH v14 11/14] qemu-img: Specify backing file for commit

[Max Reitz <mreitz@redhat.com>]

On 2014-10-24 at 18:23, Eric Blake wrote:
> On 10/24/2014 07:57 AM, Max Reitz wrote:
>> Introduce a new parameter for qemu-img commit which may be used to
>> explicitly specify the backing file into which an image should be
>> committed if the backing chain has more than a single layer.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   qemu-img-cmds.hx |  4 ++--
>>   qemu-img.c       | 32 +++++++++++++++++++++++---------
>>   qemu-img.texi    | 12 +++++++++++-
>>   3 files changed, 36 insertions(+), 12 deletions(-)
>>
>> +If the backing chain of the given image file @var{filename} has more than one
>> +layer, the backing file into which the changes will be committed may be
>> +specified as @var{base} (which has to be part of @var{filename}'s backing
>> +chain). If @var{base} is not specified, the immediate backing file of the top
>> +image (which is @var{filename}) will be used. For reasons of consistency,
>> +explicitly specifying @var{base} will always imply @code{-d} (otherwise, an
>> +image could be committed in an indirect backing file and emptying it might lead
>> +to different data being read from it because the intermediate backing chain
>> +overrules the commit target).
> I wonder if there is any better wording to make it obvious that both
> instances of 'it' in the (comment) refer to the further reference of the
> top image, and not the closer reference to the indirect backing file. Maybe:
>
> ...always imply -d (since emptying an image after committing to an
> indirect backing file would lead to different data being read from the
> image due to content in the intermediate backing chain overruling the
> commit target)
>
> But I can live with the wording as you proposed it, so:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Seems better to me; I'll change it if I respin (I'm looking forward to 
it by now), and if for some strange reason I will not respin I would be 
fine with the maintainer changing it accordingly, too.

Max