Patchwork Qemu-img convert with -B

login
register
mail settings
Submitter Brad Campbell
Date April 28, 2011, 2:06 a.m.
Message ID <4DB8CBB8.9050002@fnarfbargle.com>
Download mbox | patch
Permalink /patch/93165/
State New
Headers show

Comments

Brad Campbell - April 28, 2011, 2:06 a.m.
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]) {
Kevin Wolf - April 28, 2011, 6:36 a.m.
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.
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

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;
+        }
      }