Hi Eric,
----- Original Message -----
> From: "Eric Blake" <ebl...@redhat.com>
> To: "Miroslav Rezanina" <mreza...@redhat.com>
> Cc: qemu-devel@nongnu.org, kw...@redhat.com, pbonz...@redhat.com,
> stefa...@redhat.com
> 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 <mreza...@redhat.com>
> > ---
> >
> > 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)
> > +@item -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
