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

[Max Reitz]

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 <address@hidden>
> > ---
> >   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 <address@hidden>

Thanks,

Max