diff mbox

Qemu-img convert with -B

Message ID 4DB8CBB8.9050002@fnarfbargle.com
State New
Headers show

Commit Message

Brad Campbell April 28, 2011, 2:06 a.m. UTC 
On 27/04/11 22:02, Brad Campbell wrote:
> On 27/04/11 21:56, Kevin Wolf wrote:
>
>> When you don't have a backing file, leaving an cluster unallocated means
>> that it's zero. When you have a backing file, it could be anything. So
>> if qemu-img convert wanted to save this space, it would have to read
>> from the backing file and leave the cluster unallocated if it reads as
>> zero.
>>
>> This is something that qemu-img doesn't do today.
>

This passes cursory testing, but I'm just wondering if this is along the 
right lines?

      /* Check if compression is supported */
@@ -767,6 +774,9 @@ static int img_convert(int argc, char **argv)
      bs_offset = 0;
      bdrv_get_geometry(bs[0], &bs_sectors);
      buf = qemu_malloc(IO_BUF_SIZE);
+    if (out_baseimg) {
+        buf3 = qemu_malloc(IO_BUF_SIZE);
+    }

      if (compress) {
          ret = bdrv_get_info(out_bs, &bdi);
@@ -889,9 +899,17 @@ static int img_convert(int argc, char **argv)
                     are present in both the output's and input's base 
images (no
                     need to copy them). */
                  if (out_baseimg) {
-                    if (!bdrv_is_allocated(bs[bs_i], sector_num - 
bs_offset,
-                                           n, &n1)) {
+                    if (bdrv_read(bs[bs_i], sector_num - bs_offset, 
buf, n) < 0) {
+                        error_report("error while reading input file");
+                        goto out;
+                    }
+                    if (bdrv_read(out_bf, sector_num - bs_offset, buf3, 
n) < 0) {
+                        error_report("error while reading backing file");
+                        goto out;
+                    }
+                    if (!compare_sectors(buf, buf3, n, &n1)) {
                          sector_num += n1;
+    //                    printf("Skipping %u sectors\n",n1);
                          continue;
                      }
                      /* The next 'n1' sectors are allocated in the 
input image. Copy
@@ -939,9 +957,16 @@ out:
      free_option_parameters(create_options);
      free_option_parameters(param);
      qemu_free(buf);
+    if (buf3) {
+        qemu_free(buf3);
+    }
      if (out_bs) {
          bdrv_delete(out_bs);
      }
+    if (out_bf) {
+        bdrv_delete(out_bf);
+    }
+
      if (bs) {
          for (bs_i = 0; bs_i < bs_n; bs_i++) {
              if (bs[bs_i]) {

Comments

Kevin Wolf April 28, 2011, 6:36 a.m. UTC | #1 
Am 28.04.2011 04:06, schrieb Brad Campbell:
> On 27/04/11 22:02, Brad Campbell wrote:
>> On 27/04/11 21:56, Kevin Wolf wrote:
>>
>>> When you don't have a backing file, leaving an cluster unallocated means
>>> that it's zero. When you have a backing file, it could be anything. So
>>> if qemu-img convert wanted to save this space, it would have to read
>>> from the backing file and leave the cluster unallocated if it reads as
>>> zero.
>>>
>>> This is something that qemu-img doesn't do today.
>>
> 
> This passes cursory testing, but I'm just wondering if this is along the 
> right lines?

I haven't checked all details, but it looks like what I would have done.
(Though something is wrong with your indentations, I suspect that the
patch wouldn't apply)

> @@ -939,9 +957,16 @@ out:
>       free_option_parameters(create_options);
>       free_option_parameters(param);
>       qemu_free(buf);
> +    if (buf3) {
> +        qemu_free(buf3);
> +    }

qemu_free (and the libc free, too) work just fine with NULL, so the
check isn't needed.

Kevin
Brad Campbell April 28, 2011, 9:38 a.m. UTC | #2 
On 28/04/11 14:36, Kevin Wolf wrote:
> Am 28.04.2011 04:06, schrieb Brad Campbell:
>> On 27/04/11 22:02, Brad Campbell wrote:
>>> On 27/04/11 21:56, Kevin Wolf wrote:
>>>
>>>> When you don't have a backing file, leaving an cluster unallocated means
>>>> that it's zero. When you have a backing file, it could be anything. So
>>>> if qemu-img convert wanted to save this space, it would have to read
>>>> from the backing file and leave the cluster unallocated if it reads as
>>>> zero.
>>>>
>>>> This is something that qemu-img doesn't do today.
>> This passes cursory testing, but I'm just wondering if this is along the
>> right lines?
> I haven't checked all details, but it looks like what I would have done.
> (Though something is wrong with your indentations, I suspect that the
> patch wouldn't apply)
>

Odd, I generated it with git diff. Must have lost something in the copy & paste from the terminal.

I'm using tabs=spaces and a ts of 4 in vim. It seemed to fit in with the rest of the file.

>> @@ -939,9 +957,16 @@ out:
>>        free_option_parameters(create_options);
>>        free_option_parameters(param);
>>        qemu_free(buf);
>> +    if (buf3) {
>> +        qemu_free(buf3);
>> +    }
> qemu_free (and the libc free, too) work just fine with NULL, so the
> check isn't needed.

I ran some more tests on it and there are some small issues I need to fix up, but it does what it 
says on the tin.

Cheers for the feedback, I'll do some cleanups and prepare something for submission.

Regards,
Brad
diff mbox

Patch

diff --git a/qemu-img.c b/qemu-img.c
index d9c2c12..ab4c70c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1,4 +1,4 @@ 
-/*
+ /*
   * QEMU disk image utility
   *
   * Copyright (c) 2003-2008 Fabrice Bellard
@@ -571,11 +571,12 @@  static int img_convert(int argc, char **argv)
      int progress = 0;
      const char *fmt, *out_fmt, *out_baseimg, *out_filename;
      BlockDriver *drv, *proto_drv;
-    BlockDriverState **bs = NULL, *out_bs = NULL;
+    BlockDriverState **bs = NULL, *out_bs = NULL, *out_bf = NULL;
      int64_t total_sectors, nb_sectors, sector_num, bs_offset;
      uint64_t bs_sectors;
      uint8_t * buf = NULL;
      const uint8_t *buf1;
+    uint8_t * buf3 = NULL;
      BlockDriverInfo bdi;
      QEMUOptionParameter *param = NULL, *create_options = NULL;
      QEMUOptionParameter *out_baseimg_param;
@@ -719,6 +720,12 @@  static int img_convert(int argc, char **argv)
      out_baseimg_param = get_option_parameter(param, 
BLOCK_OPT_BACKING_FILE);
      if (out_baseimg_param) {
          out_baseimg = out_baseimg_param->value.s;
+       out_bf = bdrv_new_open(out_baseimg, NULL, BDRV_O_FLAGS);
+       if (!out_bf) {
+            error_report("Could not open backing file '%s'", out_baseimg);
+            ret = -1;
+            goto out;
+        }
      }