[Qemu-devel] [PATCH] qemu-img: refuse to compress files with invalid size

[Qemu-devel] [PATCH] qemu-img: refuse to compress files with invalid size

[Stefan Hajnoczi]

Image file compression works at cluster granularity.  It is not possible
to compress less than a cluster of data at a time.

Print an error when attempting qemu-img convert -c with an input file
that is not a multiple of the cluster size.

I considered automatically adjusting the output file size but think it's
better to be explicit.  This avoids confusion when users notice that
image file size changed after conversion.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qemu-img.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index 31627b0..2273851 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1370,6 +1370,13 @@ static int img_convert(int argc, char **argv)
             goto out;
         }
         cluster_sectors = cluster_size >> 9;
+        if (total_sectors % cluster_sectors) {
+            error_report("compression requires that input file size is a "
+                         "multiple of %d bytes",
+                         cluster_size);
+            ret = -1;
+            goto out;
+        }
         sector_num = 0;
 
         nb_sectors = total_sectors;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] qemu-img: refuse to compress files with invalid size

[Benoît Canet]

Le Friday 12 Apr 2013 à 19:37:48 (+0200), Stefan Hajnoczi a écrit :
> Image file compression works at cluster granularity.  It is not possible
> to compress less than a cluster of data at a time.
> 
> Print an error when attempting qemu-img convert -c with an input file
> that is not a multiple of the cluster size.
> 
> I considered automatically adjusting the output file size but think it's
> better to be explicit.  This avoids confusion when users notice that
> image file size changed after conversion.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  qemu-img.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 31627b0..2273851 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1370,6 +1370,13 @@ static int img_convert(int argc, char **argv)
>              goto out;
>          }
>          cluster_sectors = cluster_size >> 9;
> +        if (total_sectors % cluster_sectors) {
> +            error_report("compression requires that input file size is a "
> +                         "multiple of %d bytes",
> +                         cluster_size);
> +            ret = -1;
> +            goto out;
> +        }
>          sector_num = 0;
>  
>          nb_sectors = total_sectors;
> -- 
> 1.8.1.4
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH] qemu-img: refuse to compress files with invalid size

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1091 bytes --]

On 04/12/2013 01:55 PM, Benoît Canet wrote:
> Le Friday 12 Apr 2013 à 19:37:48 (+0200), Stefan Hajnoczi a écrit :
>> Image file compression works at cluster granularity.  It is not possible
>> to compress less than a cluster of data at a time.

Shouldn't it be possible to treat a file that is not rounded to a
cluster boundary as though the rest of the final cluster is all NUL
bytes, and compress that?  As long as you know the original size, this
operation is reversible, instead of having to error out just because the
original wasn't a complete multiple.

>> I considered automatically adjusting the output file size but think it's
>> better to be explicit.  This avoids confusion when users notice that
>> image file size changed after conversion.

I agree that changing the final image size on a round trip through
compression is not appropriate, but does that mean we have to reject the
file completely if it wasn't sized to a multiple to begin with?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH] qemu-img: refuse to compress files with invalid size

[Stefan Hajnoczi]

On Fri, Apr 12, 2013 at 02:04:41PM -0600, Eric Blake wrote:
> On 04/12/2013 01:55 PM, Benoît Canet wrote:
> > Le Friday 12 Apr 2013 à 19:37:48 (+0200), Stefan Hajnoczi a écrit :
> >> Image file compression works at cluster granularity.  It is not possible
> >> to compress less than a cluster of data at a time.
> 
> Shouldn't it be possible to treat a file that is not rounded to a
> cluster boundary as though the rest of the final cluster is all NUL
> bytes, and compress that?  As long as you know the original size, this
> operation is reversible, instead of having to error out just because the
> original wasn't a complete multiple.
> 
> >> I considered automatically adjusting the output file size but think it's
> >> better to be explicit.  This avoids confusion when users notice that
> >> image file size changed after conversion.
> 
> I agree that changing the final image size on a round trip through
> compression is not appropriate, but does that mean we have to reject the
> file completely if it wasn't sized to a multiple to begin with?

I will send a new patch that does this.  kwolf also suggested it on
Friday and it sounds like a good idea.

Stefan