Re: [Qemu-devel] [PATCH 4/5] qemu-img: add compressed clusters to BlockFragInfo

[Stefan Hajnoczi]

On Tue, Feb 05, 2013 at 03:27:10PM -0700, Eric Blake wrote:
> On 02/05/2013 11:54 AM, Stefan Hajnoczi wrote:
> > Show how many clusters are compressed.  This can be used to monitor how
> > many compressed clusters remain and whether to recompress the image.
> > 
> > Suggested-by: Cole Robinson <address@hidden>
> > Signed-off-by: Stefan Hajnoczi <address@hidden>
> > ---
> 
> > +++ b/qemu-img.c
> > @@ -471,10 +471,11 @@ static int img_check(int argc, char **argv)
> >  
> >      if (bfi->total_clusters != 0 && bfi->allocated_clusters != 0) {
> >          printf("%" PRId64 "/%" PRId64 "= %0.2f%% allocated, "
> > -               "%0.2f%% fragmented\n",
> > +               "%0.2f%% fragmented, %0.2f%% compressed\n",
> 
> That might be misleading output.  Stating 90% fragmented makes sense (90
> percent of my allocations are disjoint), but stating 90% compressed has
> two possible interpretations (of the unknown number of clusters that
> were compressed, I got an impressive 90% compression ratio; vs. 90% of
> the clusters are compressed, but no idea what compression ratio that
> actually gave me).  Obviously, you meant the latter interpretation (we
> aren't telling the percentage of saved disk space due to compression,
> merely how much of the disk has been compressed).  Maybe "%0.2f%% of
> clusters use compression" would more accurately express things, but it
> ends up being longer.  Any other thoughts on avoiding an ambiguity?

Let's change it to "%0.2f%% compressed clusters".  I think that
indicates we're talking about the percentage of clusters that are
compressed, not the compression ratio.

> > +++ b/tests/qemu-iotests/common.rc
> > @@ -161,9 +161,9 @@ _cleanup_test_img()
> >  
> >  _check_test_img()
> >  {
> > -    $QEMU_IMG check -f $IMGFMT $TEST_IMG 2>&1 | \
> > -        grep -v "fragmented$" | \
> > -           sed -e 's/qemu-img\: This image format does not support 
> > checks/No errors were found on the image./'
> > +    $QEMU_IMG check "$@" -f $IMGFMT $TEST_IMG 2>&1 | \
> > +        grep -v -e "compressed$" | \
> 
> Did you really intend to lose the filtering of 'fragmented$'?

Yes.

The '$' is the hint - this command filters out the entire fragmentation
info line.  But this regex no longer matches once we added the
compressed percentage to the output.  We now match against 'compressed$'
at end-of-line.

I have replaced it with a longer regex that matches "allocated",
"fragmented", and "compressed".  This way it's clear we're trying to
filter out the entire fragmentation info line.

> > +        sed -e 's/qemu-img\: This image format does not support checks/No 
> > errors were found on the image./'
> 
> I think I've mentioned this on other pending patches that touch this
> same function...
> /me goes looking
> https://lists.gnu.org/archive/html/qemu-devel/2013-01/msg05364.html
> 
> 'grep -v | sed' is slightly inefficient compared to:
> 
> sed -e '/compressed$/d' \
>     -e 's/qemu-img:.../'
> 
> \: is not portable sed, so we should be fixing it up as long as we are
> touching this.

I only converted the tab to 4 spaces on this line but I'm happy to
squash the grep and sed invocations into a single sed invocation.

Stefan