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

[Eric Blake]

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?

> +++ 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$'?

> +        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.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature