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 <crobi...@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
> > ---
>
> > +++ 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
