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

[Miroslav Rezanina]

Hi Eric,
----- Original Message -----
> From: "Eric Blake" <address@hidden>
> To: "Miroslav Rezanina" <address@hidden>
> Cc: address@hidden, address@hidden, address@hidden, address@hidden
> Sent: Friday, February 8, 2013 6:07:35 PM
> Subject: Re: [PATCH 2/4] qemu-img: Add "Quiet mode" option
> 
> On 02/08/2013 01:33 AM, Miroslav Rezanina wrote:
> > There can be a need to turn output to stdout off. This patch adds a
> > -q option
> > that enable "Quiet mode". In Quiet mode, only errors are printed
> > out.
> > 
> > Signed-off-by: Miroslav Rezanina <address@hidden>
> > ---
> >  
> >      if (result.bfi.total_clusters != 0 &&
> >      result.bfi.allocated_clusters != 0) {
> > -        printf("%" PRId64 "/%" PRId64 "= %0.2f%% allocated,
> > %0.2f%% fragmented\n",
> > +        /* BEWARE: parameter list not indented due to long
> > expressions */
> 
> This code is being touched in an independent patch, with a solution
> much
> nicer than adding a BEWARE comment:
> https://lists.gnu.org/archive/html/qemu-devel/2013-02/msg01101.html
> 
Yeah, you're right. Unfortunatelly, this patch was sent after this one was 
prepared. I agree, that
my solution is not best but I do not want to do any unnecessary change in code.

> > +        qprintf(quiet,
> > +        "%" PRId64 "/%" PRId64 "= %0.2f%% allocated, %0.2f%%
> > fragmented\n",
> >          result.bfi.allocated_clusters, result.bfi.total_clusters,
> >          result.bfi.allocated_clusters * 100.0 /
> >          result.bfi.total_clusters,
> >          result.bfi.fragmented_clusters * 100.0 /
> >          result.bfi.allocated_clusters);
> > +        /* end of parameter list */
> >      }
> 
> This /* end of parameter list */ comment is bogus.
> 
> > @@ -742,9 +775,16 @@ static int img_convert(int argc, char **argv)
> >          case 't':
> >              cache = optarg;
> >              break;
> > +        case 'q':
> > +            quiet = true;
> > +            break;
> >          }
> >      }
> >  
> > +    if (quiet) {
> > +        progress = 0;
> > +    }
> 
> I'm still not sure I buy this.  Since '-p' normally adds output, and
> '-q' normally removes output, I'm wondering if it is better to follow
> conventions of other apps like ls, when when faced with mutually
> competing options will favor the last one specified.  Something like:
> 
> qemu-img convert -p -q => turns off -p with quiet results
> qemu-img convert -q -p => turns off -q, with progress bar results
> 
> Or, it might be worth actually erroring out if both -p and -q are
> present, in any order.
> 

I do not like when order of the "-" parameters matter, so I'm not going
to implement this behavior. I could live with error for this combination.
However, I still feel that logic "-p adds something to output and -q
turns output off" is correct. "-p" does not activate output, just improve it.
  

> > +++ 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
> > progress bar
> > +in case both @var{-q} and  @var{-p} options are used.
> 
> But at least your documentation matches your code, so I guess I can
> live
> with your choice that -q is always more powerful than -p, no matter
> what
> order they appear in.
> 
> You have an extra space before the second @var.
> 
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 
> 

-- 
Miroslav Rezanina
Software Engineer - Virtualization Team