Patchwork qemu-img: refuse to compress files with invalid size

login
register
mail settings
Submitter Stefan Hajnoczi
Date April 12, 2013, 5:37 p.m.
Message ID <1365788268-24787-1-git-send-email-stefanha@redhat.com>
Download mbox | patch
Permalink /patch/236167/
State New
Headers show

Comments

Stefan Hajnoczi - April 12, 2013, 5:37 p.m.
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(+)
Benoît Canet - April 12, 2013, 7:55 p.m.
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>
Eric Blake - April 12, 2013, 8:04 p.m.
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?
Stefan Hajnoczi - April 15, 2013, 6:53 a.m.
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

Patch

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;