Re: [Qemu-devel] [PATCH v7 2/4] qemu-img: Add "Quiet mode" option

[Miroslav Rezanina]

Hi Eric,
thanks for review, reply inline.

----- Original Message -----
> From: "Eric Blake" <address@hidden>
> To: address@hidden
> Cc: address@hidden, address@hidden, address@hidden, address@hidden
> Sent: Wednesday, December 19, 2012 7:06:35 PM
> Subject: Re: [Qemu-devel] [PATCH v7 2/4] qemu-img: Add "Quiet mode" option
> 
> On 12/17/2012 06:39 AM, address@hidden wrote:
> > From: Miroslav Rezanina <address@hidden>
> 
> Your git send-email settings threaded each message as a reply to the
> other, rather than the more typical setting of threading messages
> only
> as a reply to the cover letter.  You may want to do 'git config
> format.thread shallow' rather than your current deep approach.

I use temporary machine for sending patches as my usual one was not usable
for me in this case, so setting can be incorrect. I will take better care of 
this
next time.

> 
> > 
> > There can be need to turn output to stdout off. This patch adds a
> > -q option that
> 
> s/be need/be a need/
> 
> > enable "Quiet mode". In Quiet mode, only errors are printed out.
> > 
> > Signed-off-by: Miroslav Rezanina <address@hidden>
> > ---
> >  block.c          |  11 +++---
> >  block.h          |   2 +-
> >  blockdev.c       |   6 ++--
> >  qemu-img-cmds.hx |  28 +++++++--------
> >  qemu-img.c       | 108
> >  +++++++++++++++++++++++++++++++++++++++++--------------
> >  qemu-img.texi    |   3 ++
> >  6 files changed, 108 insertions(+), 50 deletions(-)
> > 
> > @@ -1275,7 +1275,7 @@ void qmp_drive_mirror(const char *device,
> > const char *target,
> >              ret = bdrv_img_create(target, format,
> >                                    source->filename,
> >                                    source->drv->format_name,
> > -                                  NULL, -1, flags);
> > +                                  NULL, -1, flags,false);
> 
> Space after comma.
> 
> > @@ -10,27 +10,27 @@ STEXI
> >  ETEXI
> >  
> >  DEF("check", img_check,
> > -    "check [-f fmt] [-r [leaks | all]] filename")
> > +    "check [-q] [-f fmt] [-r [leaks | all]] filename")
> 
> Is it really better to list [-q] to all of the sub-commands, or would
> it
> be easier to simply declare that -q is a universal option that can
> appear before sub-commands, as in:
> qemu-img [-q] command [command options]
> 

-q is not useable for all commands so it is listed this ways as it is same
for other options.

> >  #ifdef _WIN32
> >  #include <windows.h>
> > @@ -86,6 +87,7 @@ static void help(void)
> >             "       rebasing in this case (useful for renaming the
> >             backing file)\n"
> >             "  '-h' with or without a command shows this help and
> >             lists the supported formats\n"
> >             "  '-p' show progress of command (only certain
> >             commands)\n"
> > +           "  '-q' Quiet mode - do not print any output (except
> > errors)\n"
> 
> s/Quiet/quiet/ for consistency with the nearby lines not starting
> with a
> capital.

I use Quiet mode as name with capital Q, so I have to choose what consistency
to break - I choose the start of the description. I have no problem with using
quiet instead of Quiet.

> 
> > +++ b/qemu-img.texi
> > @@ -54,6 +54,9 @@ indicates that target image must be compressed
> > (qcow format only)
> >  with or without a command shows help and lists the supported
> >  formats
> >  @item -p
> >  display progress bar (convert and rebase commands only)
> > address@hidden -q
> > +Quiet mode - do not print any output (except errors). There's no
> > progres bar
> 
> s/progres/progress/
> 
> > +in case both @var{-q} and  @var{-p} options are used.
> 
> Should it be a hard error if both -q and -p are given?  Otherwise,
> when
> dealing with conflicting options, it's more typical to have the
> semantic
> of last one wins: 'qemu-img -q -p' prints progress, and only
> 'qemu-img
> -p -q' is quiet.
> 

Depends on point of view - There's no conflict for -p and -q as
-p adds progress bar to output and -q suppress output. You can imagine 
(in theory) -q as redirecting stdout to /dev/null. 
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 
>