Re: [Qemu-devel] [PATCH v2 5/6] qemu-img: Specify backing file for commit

[Max Reitz <mreitz@redhat.com>]

On 10.04.2014 11:05, Fam Zheng wrote:
> On Tue, 04/08 14:50, Max Reitz wrote:
>> Introduce a new parameter for qemu-img commit which may be used to
>> explicitly specify the backing file unto 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       | 22 +++++++++++++++-------
>>   qemu-img.texi    |  8 +++++++-
>>   3 files changed, 24 insertions(+), 10 deletions(-)
>>
>> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
>> index 8bc55cd..7f62f6d 100644
>> --- a/qemu-img-cmds.hx
>> +++ b/qemu-img-cmds.hx
>> @@ -22,9 +22,9 @@ STEXI
>>   ETEXI
>>   
>>   DEF("commit", img_commit,
>> -    "commit [-q] [-f fmt] [-t cache] [-p] filename")
>> +    "commit [-q] [-f fmt] [-t cache] [-b backing_file] [-p] filename")
>>   STEXI
>> -@item commit [-q] [-f @var{fmt}] [-t @var{cache}] [-p] @var{filename}
>> +@item commit [-q] [-f @var{fmt}] [-t @var{cache}] [-b @var{backing_file}] [-p] @var{filename}
>>   ETEXI
>>   
>>   DEF("compare", img_compare,
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 0a9eff7..9d4bdbc 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -725,15 +725,16 @@ static void run_block_job(BlockJob *job, Error **errp)
>>   static int img_commit(int argc, char **argv)
>>   {
>>       int c, ret, flags;
>> -    const char *filename, *fmt, *cache;
>> +    const char *filename, *fmt, *cache, *base;
>>       BlockDriverState *bs, *base_bs;
>>       bool progress = false, quiet = false;
>>       Error *local_err = NULL;
>>   
>>       fmt = NULL;
>>       cache = BDRV_DEFAULT_CACHE;
>> +    base = NULL;
>>       for(;;) {
>> -        c = getopt(argc, argv, "f:ht:qp");
>> +        c = getopt(argc, argv, "f:ht:b:qp");
>>           if (c == -1) {
>>               break;
>>           }
>> @@ -748,6 +749,9 @@ static int img_commit(int argc, char **argv)
>>           case 't':
>>               cache = optarg;
>>               break;
>> +        case 'b':
>> +            base = optarg;
>> +            break;
>>           case 'p':
>>               progress = true;
>>               break;
>> @@ -782,12 +786,16 @@ static int img_commit(int argc, char **argv)
>>       qemu_progress_init(progress, 1.f);
>>       qemu_progress_print(0.f, 100);
>>   
>> -    /* This is different from QMP, which by default uses the deepest file in the
>> -     * backing chain (i.e., the very base); however, the traditional behavior of
>> -     * qemu-img commit is using the immediate backing file. */
>> -    base_bs = bs->backing_hd;
>> +    if (base) {
>> +        base_bs = bdrv_find_backing_image(bs, base);
>> +    } else {
>> +        /* This is different from QMP, which by default uses the deepest file in
>> +         * the backing chain (i.e., the very base); however, the traditional
>> +         * behavior of qemu-img commit is using the immediate backing file. */
>> +        base_bs = bs->backing_hd;
>> +    }
>>       if (!base_bs) {
>> -        error_set(&local_err, QERR_BASE_NOT_FOUND, "NULL");
>> +        error_set(&local_err, QERR_BASE_NOT_FOUND, base ?: "NULL");
>>           goto done;
>>       }
>>   
>> diff --git a/qemu-img.texi b/qemu-img.texi
>> index 1a9c08f..4a9f493 100644
>> --- a/qemu-img.texi
>> +++ b/qemu-img.texi
>> @@ -140,7 +140,7 @@ this case. @var{backing_file} will never be modified unless you use the
>>   The size can also be specified using the @var{size} option with @code{-o},
>>   it doesn't need to be specified separately in this case.
>>   
>> -@item commit [-q] [-f @var{fmt}] [-t @var{cache}] [-p] @var{filename}
>> +@item commit [-q] [-f @var{fmt}] [-t @var{cache}] [-b @var{backing_file}] [-p] @var{filename}
>>   
>>   Commit the changes recorded in @var{filename} in its base image or backing file.
>>   If the backing file is smaller than the snapshot, then the backing file will be
>> @@ -149,6 +149,12 @@ the backing file, the backing file will not be truncated.  If you want the
>>   backing file to match the size of the smaller snapshot, you can safely truncate
>>   it yourself once the commit operation successfully completes.
>>   
>> +If the backing chain of the given image file @var{filename} has more than one
>> +layer, the backing file unto which the changes shall be committed may be
>> +specified as @var{backing_file} (which has to be part of @var{filename}'s
>> +backing chain). If @var{filename} is not specified, the immediate backing file
> s/@var{filename}/@var{backing_file}/ ?

Right.

> BTW how about just calling it 'base' as in qmp commands, because backing_file
> has usages in (slightly) different context of create.

I just called it “backing_file”, as there are currently no qemu-img 
commands with a “base” parameter; then again, there aren't any qemu-img 
commands for which a filename of the backing chain may be specified, so 
you're correct.

Max

> Other than the two questions,
>
> Reviewed-by: Fam Zheng <famz@redhat.com>
>
>> +of the top image (which is @var{filename}) will be used.
>> +
>>   @item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-s] [-q] @var{filename1} @var{filename2}
>>   
>>   Check if two images have the same content. You can compare images with
>> -- 
>> 1.9.1
>>
>>