Re: [Qemu-devel] [PATCH 1/2] qemu-img: Allow source cache mode specification

[Max Reitz <mreitz@redhat.com>]

On 21.07.2014 17:52, Eric Blake wrote:
> On 07/19/2014 02:35 PM, Max Reitz wrote:
>> Many qemu-img subcommands only read the source file(s) once. For these
>> use cases, a full write-back cache is unnecessary and mainly clutters
>> host cache memory. Though this is generally no concern as cache memory
>> is freely available and can be scaled by the host OS, it may become a
>> concern with thin provisioning.
>>
>> For these cases, it makes sense to allow users to freely specify the
>> source cache mode (e.g. use no cache at all).
>>
>> This commit adds a new switch (-T) for the qemu-img subcommands check,
>> compare, convert and rebase to specify the cache to be used for source
>> images (the backing file in case of rebase).
> What mnemonic did you have in mind when choosing -T? Or was it just a
> universally available letter for the subcommands you were touching?

To be honest, I just didn't know what -t stands for. Therefore I just 
thought it might be remotely logical if the lower-cased letter is used 
for destination and the upper-cased letter for source caching.

>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   qemu-img-cmds.hx | 16 ++++++------
>>   qemu-img.c       | 78 ++++++++++++++++++++++++++++++++++++++++++++------------
>>   qemu-img.texi    | 14 +++++++---
>>   3 files changed, 80 insertions(+), 28 deletions(-)
>>
>> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
>> index d029609..5613628 100644
>> --- a/qemu-img-cmds.hx
>> +++ b/qemu-img-cmds.hx
>> @@ -10,9 +10,9 @@ STEXI
>>   ETEXI
>>   
>>   DEF("check", img_check,
>> -    "check [-q] [-f fmt] [--output=ofmt]  [-r [leaks | all]] filename")
>> +    "check [-q] [-f fmt] [--output=ofmt]  [-r [leaks | all]] [-T src_cache] filename")
> Might be nice to fix the unintentional double space before -r while
> touching this line.

Okay, I'll send a v2 (taking into account your comments for patch 2).

>>   DEF("convert", img_convert,
>> -    "convert [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] filename [filename2 [...]] output_filename")
>> +    "convert [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] filename [filename2 [...]] output_filename")
> Oh, maybe you just picked -T for source because -t was already picked
> for destination?

Right. :-)

> At any rate, seems reasonable.
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks,

Max