Message ID | 4DB8CBB8.9050002@fnarfbargle.com |
---|---|
State | New |
Headers | show |
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
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 --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; + } }