| Message ID | 1470211278-22868-1-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 03/08/2016 10:01, Fam Zheng wrote: > dmg.o was moved to block-obj-m in 5505e8b76 to become a separate module, > so that its reference to libbz2, since 6b383c08c, doesn't add an extra > library to the main executable. > > We are working on on-demand loading of block drivers which will be > easier if all format drivers are always built-in (so that the format > probing is not a egg-and-chicken problem). > > dmg is the only modularized format driver for now. For above reason, we > want to move it back to block-obj-y, as is done in this patch. > > The bz2 uncompressing code that requires bzip2 library is kept in a > separate function in the added dmg-bz2.o, which will be loaded at > program start in bdrv_init() as usual. It fills in the function pointer > inside dmg.o, so that bz2 uncompress can be done in reading path. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block/Makefile.objs | 6 ++--- > block/dmg-bz2.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++ > block/dmg.c | 68 +++++++++++++---------------------------------------- > block/dmg.h | 59 ++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 140 insertions(+), 55 deletions(-) > create mode 100644 block/dmg-bz2.c > create mode 100644 block/dmg.h > > diff --git a/block/Makefile.objs b/block/Makefile.objs > index 2593a2f..8a8a48e 100644 > --- a/block/Makefile.objs > +++ b/block/Makefile.objs > @@ -1,4 +1,4 @@ > -block-obj-y += raw_bsd.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o > +block-obj-y += raw_bsd.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o dmg.o > block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o > block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o > block-obj-y += qed-check.o > @@ -39,7 +39,7 @@ gluster.o-libs := $(GLUSTERFS_LIBS) > ssh.o-cflags := $(LIBSSH2_CFLAGS) > ssh.o-libs := $(LIBSSH2_LIBS) > archipelago.o-libs := $(ARCHIPELAGO_LIBS) > -block-obj-m += dmg.o > -dmg.o-libs := $(BZIP2_LIBS) > +block-obj-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o You can make CONFIG_BZIP2=m in configure and simplify this line. Otherwise, Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > +dmg-bz2.o-libs := $(BZIP2_LIBS) > qcow.o-libs := -lz > linux-aio.o-libs := -laio > diff --git a/block/dmg-bz2.c b/block/dmg-bz2.c > new file mode 100644 > index 0000000..a60e398 > --- /dev/null > +++ b/block/dmg-bz2.c > @@ -0,0 +1,62 @@ > +/* > + * DMG bzip2 uncompression > + * > + * Copyright (c) 2004 Johannes E. Schindelin > + * Copyright (c) 2016 Red Hat, Int. > + * > + * Permission is hereby granted, free of charge, to any person obtaining a copy > + * of this software and associated documentation files (the "Software"), to deal > + * in the Software without restriction, including without limitation the rights > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell > + * copies of the Software, and to permit persons to whom the Software is > + * furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice shall be included in > + * all copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > + * THE SOFTWARE. > + */ > +#include "qemu/osdep.h" > +#include "qemu-common.h" > +#include "dmg.h" > +#include <bzlib.h> > + > +static int dmg_uncompress_bz2_do(char *next_in, unsigned int avail_in, > + char *next_out, unsigned int avail_out) > +{ > + int ret; > + uint64_t total_out; > + bz_stream bzstream = {}; > + > + ret = BZ2_bzDecompressInit(&bzstream, 0, 0); > + if (ret != BZ_OK) { > + return -1; > + } > + bzstream.next_in = next_in; > + bzstream.avail_in = avail_in; > + bzstream.next_out = next_out; > + bzstream.avail_out = avail_out; > + ret = BZ2_bzDecompress(&bzstream); > + total_out = ((uint64_t)bzstream.total_out_hi32 << 32) + > + bzstream.total_out_lo32; > + BZ2_bzDecompressEnd(&bzstream); > + if (ret != BZ_STREAM_END || > + total_out != avail_out) { > + return -1; > + } > + return 0; > +} > + > +__attribute__((constructor)) > +static void dmg_bz2_init(void) > +{ > + assert(!dmg_uncompress_bz2); > + dmg_uncompress_bz2 = dmg_uncompress_bz2_do; > +} > + > diff --git a/block/dmg.c b/block/dmg.c > index b0ed89b..861d3aa 100644 > --- a/block/dmg.c > +++ b/block/dmg.c > @@ -28,10 +28,10 @@ > #include "qemu/bswap.h" > #include "qemu/error-report.h" > #include "qemu/module.h" > -#include <zlib.h> > -#ifdef CONFIG_BZIP2 > -#include <bzlib.h> > -#endif > +#include "dmg.h" > + > +int (*dmg_uncompress_bz2)(char *next_in, unsigned int avail_in, > + char *next_out, unsigned int avail_out); > > enum { > /* Limit chunk sizes to prevent unreasonable amounts of memory being used > @@ -41,31 +41,6 @@ enum { > DMG_SECTORCOUNTS_MAX = DMG_LENGTHS_MAX / 512, > }; > > -typedef struct BDRVDMGState { > - CoMutex lock; > - /* each chunk contains a certain number of sectors, > - * offsets[i] is the offset in the .dmg file, > - * lengths[i] is the length of the compressed chunk, > - * sectors[i] is the sector beginning at offsets[i], > - * sectorcounts[i] is the number of sectors in that chunk, > - * the sectors array is ordered > - * 0<=i<n_chunks */ > - > - uint32_t n_chunks; > - uint32_t* types; > - uint64_t* offsets; > - uint64_t* lengths; > - uint64_t* sectors; > - uint64_t* sectorcounts; > - uint32_t current_chunk; > - uint8_t *compressed_chunk; > - uint8_t *uncompressed_chunk; > - z_stream zstream; > -#ifdef CONFIG_BZIP2 > - bz_stream bzstream; > -#endif > -} BDRVDMGState; > - > static int dmg_probe(const uint8_t *buf, int buf_size, const char *filename) > { > int len; > @@ -210,10 +185,9 @@ static bool dmg_is_known_block_type(uint32_t entry_type) > case 0x00000001: /* uncompressed */ > case 0x00000002: /* zeroes */ > case 0x80000005: /* zlib */ > -#ifdef CONFIG_BZIP2 > - case 0x80000006: /* bzip2 */ > -#endif > return true; > + case 0x80000006: /* bzip2 */ > + return !!dmg_uncompress_bz2; > default: > return false; > } > @@ -587,9 +561,6 @@ static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num) > if (!is_sector_in_chunk(s, s->current_chunk, sector_num)) { > int ret; > uint32_t chunk = search_chunk(s, sector_num); > -#ifdef CONFIG_BZIP2 > - uint64_t total_out; > -#endif > > if (chunk >= s->n_chunks) { > return -1; > @@ -620,8 +591,10 @@ static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num) > return -1; > } > break; } > -#ifdef CONFIG_BZIP2 > case 0x80000006: /* bzip2 compressed */ > + if (!dmg_uncompress_bz2) { > + break; > + } > /* we need to buffer, because only the chunk as whole can be > * inflated. */ > ret = bdrv_pread(bs->file, s->offsets[chunk], > @@ -630,24 +603,15 @@ static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num) > return -1; > } > > - ret = BZ2_bzDecompressInit(&s->bzstream, 0, 0); > - if (ret != BZ_OK) { > - return -1; > - } > - s->bzstream.next_in = (char *)s->compressed_chunk; > - s->bzstream.avail_in = (unsigned int) s->lengths[chunk]; > - s->bzstream.next_out = (char *)s->uncompressed_chunk; > - s->bzstream.avail_out = (unsigned int) 512 * s->sectorcounts[chunk]; > - ret = BZ2_bzDecompress(&s->bzstream); > - total_out = ((uint64_t)s->bzstream.total_out_hi32 << 32) + > - s->bzstream.total_out_lo32; > - BZ2_bzDecompressEnd(&s->bzstream); > - if (ret != BZ_STREAM_END || > - total_out != 512 * s->sectorcounts[chunk]) { > - return -1; > + ret = dmg_uncompress_bz2((char *)s->compressed_chunk, > + (unsigned int) s->lengths[chunk], > + (char *)s->uncompressed_chunk, > + (unsigned int) > + 512 * s->sectorcounts[chunk]); > + if (ret < 0) { > + return ret; > } > break; > -#endif /* CONFIG_BZIP2 */ > case 1: /* copy */ > ret = bdrv_pread(bs->file, s->offsets[chunk], > s->uncompressed_chunk, s->lengths[chunk]); > diff --git a/block/dmg.h b/block/dmg.h > new file mode 100644 > index 0000000..b592d6f > --- /dev/null > +++ b/block/dmg.h > @@ -0,0 +1,59 @@ > +/* > + * Header for DMG driver > + * > + * Copyright (c) 2004-2006 Fabrice Bellard > + * Copyright (c) 2016 Red hat, Inc. > + * > + * Permission is hereby granted, free of charge, to any person obtaining a copy > + * of this software and associated documentation files (the "Software"), to deal > + * in the Software without restriction, including without limitation the rights > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell > + * copies of the Software, and to permit persons to whom the Software is > + * furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice shall be included in > + * all copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > + * THE SOFTWARE. > + */ > + > +#ifndef BLOCK_DMG_H > +#define BLOCK_DMG_H > + > +#include "qemu/osdep.h" > +#include "qemu-common.h" > +#include "block/block_int.h" > +#include <zlib.h> > + > +typedef struct BDRVDMGState { > + CoMutex lock; > + /* each chunk contains a certain number of sectors, > + * offsets[i] is the offset in the .dmg file, > + * lengths[i] is the length of the compressed chunk, > + * sectors[i] is the sector beginning at offsets[i], > + * sectorcounts[i] is the number of sectors in that chunk, > + * the sectors array is ordered > + * 0<=i<n_chunks */ > + > + uint32_t n_chunks; > + uint32_t *types; > + uint64_t *offsets; > + uint64_t *lengths; > + uint64_t *sectors; > + uint64_t *sectorcounts; > + uint32_t current_chunk; > + uint8_t *compressed_chunk; > + uint8_t *uncompressed_chunk; > + z_stream zstream; > +} BDRVDMGState; > + > +extern int (*dmg_uncompress_bz2)(char *next_in, unsigned int avail_in, > + char *next_out, unsigned int avail_out); > + > +#endif >
On 03.08.2016 10:01, Fam Zheng wrote: > dmg.o was moved to block-obj-m in 5505e8b76 to become a separate module, > so that its reference to libbz2, since 6b383c08c, doesn't add an extra > library to the main executable. > > We are working on on-demand loading of block drivers which will be > easier if all format drivers are always built-in (so that the format > probing is not a egg-and-chicken problem). > > dmg is the only modularized format driver for now. For above reason, we > want to move it back to block-obj-y, as is done in this patch. > > The bz2 uncompressing code that requires bzip2 library is kept in a > separate function in the added dmg-bz2.o, which will be loaded at > program start in bdrv_init() as usual. It fills in the function pointer > inside dmg.o, so that bz2 uncompress can be done in reading path. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block/Makefile.objs | 6 ++--- > block/dmg-bz2.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++ > block/dmg.c | 68 +++++++++++++---------------------------------------- > block/dmg.h | 59 ++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 140 insertions(+), 55 deletions(-) > create mode 100644 block/dmg-bz2.c > create mode 100644 block/dmg.h [...] > diff --git a/block/dmg-bz2.c b/block/dmg-bz2.c > new file mode 100644 > index 0000000..a60e398 > --- /dev/null > +++ b/block/dmg-bz2.c > @@ -0,0 +1,62 @@ > +/* > + * DMG bzip2 uncompression > + * > + * Copyright (c) 2004 Johannes E. Schindelin > + * Copyright (c) 2016 Red Hat, Int. *Inc > + * > + * Permission is hereby granted, free of charge, to any person obtaining a copy > + * of this software and associated documentation files (the "Software"), to deal > + * in the Software without restriction, including without limitation the rights > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell > + * copies of the Software, and to permit persons to whom the Software is > + * furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice shall be included in > + * all copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > + * THE SOFTWARE. > + */ > +#include "qemu/osdep.h" > +#include "qemu-common.h" > +#include "dmg.h" > +#include <bzlib.h> > + > +static int dmg_uncompress_bz2_do(char *next_in, unsigned int avail_in, > + char *next_out, unsigned int avail_out) > +{ > + int ret; > + uint64_t total_out; > + bz_stream bzstream = {}; > + > + ret = BZ2_bzDecompressInit(&bzstream, 0, 0); > + if (ret != BZ_OK) { > + return -1; > + } > + bzstream.next_in = next_in; > + bzstream.avail_in = avail_in; > + bzstream.next_out = next_out; > + bzstream.avail_out = avail_out; > + ret = BZ2_bzDecompress(&bzstream); > + total_out = ((uint64_t)bzstream.total_out_hi32 << 32) + > + bzstream.total_out_lo32; > + BZ2_bzDecompressEnd(&bzstream); > + if (ret != BZ_STREAM_END || > + total_out != avail_out) { > + return -1; > + } > + return 0; > +} > + > +__attribute__((constructor)) > +static void dmg_bz2_init(void) > +{ > + assert(!dmg_uncompress_bz2); > + dmg_uncompress_bz2 = dmg_uncompress_bz2_do; > +} > + A superfluous empty line here, which git complains about. > diff --git a/block/dmg.c b/block/dmg.c > index b0ed89b..861d3aa 100644 > --- a/block/dmg.c > +++ b/block/dmg.c [...] > @@ -630,24 +603,15 @@ static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num) > return -1; > } > > - ret = BZ2_bzDecompressInit(&s->bzstream, 0, 0); > - if (ret != BZ_OK) { > - return -1; > - } > - s->bzstream.next_in = (char *)s->compressed_chunk; > - s->bzstream.avail_in = (unsigned int) s->lengths[chunk]; > - s->bzstream.next_out = (char *)s->uncompressed_chunk; > - s->bzstream.avail_out = (unsigned int) 512 * s->sectorcounts[chunk]; > - ret = BZ2_bzDecompress(&s->bzstream); > - total_out = ((uint64_t)s->bzstream.total_out_hi32 << 32) + > - s->bzstream.total_out_lo32; > - BZ2_bzDecompressEnd(&s->bzstream); > - if (ret != BZ_STREAM_END || > - total_out != 512 * s->sectorcounts[chunk]) { > - return -1; > + ret = dmg_uncompress_bz2((char *)s->compressed_chunk, > + (unsigned int) s->lengths[chunk], > + (char *)s->uncompressed_chunk, > + (unsigned int) > + 512 * s->sectorcounts[chunk]); Pre-existing, but you're touching the code, so: The last two lines result in a uint64_t because it's just 512 that's cast to an unsigned int. Anyway, these casts look pretty superfluous to me and I don't see the reason behind them. None of the values can (supposedly) overflow, but if they did, the casts wouldn't prevent this. So they're just cruft. If we wanted to do something about possible overflows, we'd need to put assertions here. So all in all this is not wrong, but pretty ugly still. Do you want to change it? Max > + if (ret < 0) { > + return ret; > } > break; > -#endif /* CONFIG_BZIP2 */ > case 1: /* copy */ > ret = bdrv_pread(bs->file, s->offsets[chunk], > s->uncompressed_chunk, s->lengths[chunk]);
On Wed, 08/10 20:30, Max Reitz wrote: > On 03.08.2016 10:01, Fam Zheng wrote: > > dmg.o was moved to block-obj-m in 5505e8b76 to become a separate module, > > so that its reference to libbz2, since 6b383c08c, doesn't add an extra > > library to the main executable. > > > > We are working on on-demand loading of block drivers which will be > > easier if all format drivers are always built-in (so that the format > > probing is not a egg-and-chicken problem). > > > > dmg is the only modularized format driver for now. For above reason, we > > want to move it back to block-obj-y, as is done in this patch. > > > > The bz2 uncompressing code that requires bzip2 library is kept in a > > separate function in the added dmg-bz2.o, which will be loaded at > > program start in bdrv_init() as usual. It fills in the function pointer > > inside dmg.o, so that bz2 uncompress can be done in reading path. > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > > block/Makefile.objs | 6 ++--- > > block/dmg-bz2.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++ > > block/dmg.c | 68 +++++++++++++---------------------------------------- > > block/dmg.h | 59 ++++++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 140 insertions(+), 55 deletions(-) > > create mode 100644 block/dmg-bz2.c > > create mode 100644 block/dmg.h > > [...] > > > diff --git a/block/dmg-bz2.c b/block/dmg-bz2.c > > new file mode 100644 > > index 0000000..a60e398 > > --- /dev/null > > +++ b/block/dmg-bz2.c > > @@ -0,0 +1,62 @@ > > +/* > > + * DMG bzip2 uncompression > > + * > > + * Copyright (c) 2004 Johannes E. Schindelin > > + * Copyright (c) 2016 Red Hat, Int. > > *Inc > > > + * > > + * Permission is hereby granted, free of charge, to any person obtaining a copy > > + * of this software and associated documentation files (the "Software"), to deal > > + * in the Software without restriction, including without limitation the rights > > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell > > + * copies of the Software, and to permit persons to whom the Software is > > + * furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice shall be included in > > + * all copies or substantial portions of the Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, > > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > > + * THE SOFTWARE. > > + */ > > +#include "qemu/osdep.h" > > +#include "qemu-common.h" > > +#include "dmg.h" > > +#include <bzlib.h> > > + > > +static int dmg_uncompress_bz2_do(char *next_in, unsigned int avail_in, > > + char *next_out, unsigned int avail_out) > > +{ > > + int ret; > > + uint64_t total_out; > > + bz_stream bzstream = {}; > > + > > + ret = BZ2_bzDecompressInit(&bzstream, 0, 0); > > + if (ret != BZ_OK) { > > + return -1; > > + } > > + bzstream.next_in = next_in; > > + bzstream.avail_in = avail_in; > > + bzstream.next_out = next_out; > > + bzstream.avail_out = avail_out; > > + ret = BZ2_bzDecompress(&bzstream); > > + total_out = ((uint64_t)bzstream.total_out_hi32 << 32) + > > + bzstream.total_out_lo32; > > + BZ2_bzDecompressEnd(&bzstream); > > + if (ret != BZ_STREAM_END || > > + total_out != avail_out) { > > + return -1; > > + } > > + return 0; > > +} > > + > > +__attribute__((constructor)) > > +static void dmg_bz2_init(void) > > +{ > > + assert(!dmg_uncompress_bz2); > > + dmg_uncompress_bz2 = dmg_uncompress_bz2_do; > > +} > > + > > A superfluous empty line here, which git complains about. > > > diff --git a/block/dmg.c b/block/dmg.c > > index b0ed89b..861d3aa 100644 > > --- a/block/dmg.c > > +++ b/block/dmg.c > > [...] > > > @@ -630,24 +603,15 @@ static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num) > > return -1; > > } > > > > - ret = BZ2_bzDecompressInit(&s->bzstream, 0, 0); > > - if (ret != BZ_OK) { > > - return -1; > > - } > > - s->bzstream.next_in = (char *)s->compressed_chunk; > > - s->bzstream.avail_in = (unsigned int) s->lengths[chunk]; > > - s->bzstream.next_out = (char *)s->uncompressed_chunk; > > - s->bzstream.avail_out = (unsigned int) 512 * s->sectorcounts[chunk]; > > - ret = BZ2_bzDecompress(&s->bzstream); > > - total_out = ((uint64_t)s->bzstream.total_out_hi32 << 32) + > > - s->bzstream.total_out_lo32; > > - BZ2_bzDecompressEnd(&s->bzstream); > > - if (ret != BZ_STREAM_END || > > - total_out != 512 * s->sectorcounts[chunk]) { > > - return -1; > > + ret = dmg_uncompress_bz2((char *)s->compressed_chunk, > > + (unsigned int) s->lengths[chunk], > > + (char *)s->uncompressed_chunk, > > + (unsigned int) > > + 512 * s->sectorcounts[chunk]); > > Pre-existing, but you're touching the code, so: The last two lines > result in a uint64_t because it's just 512 that's cast to an unsigned int. > > Anyway, these casts look pretty superfluous to me and I don't see the > reason behind them. None of the values can (supposedly) overflow, but if > they did, the casts wouldn't prevent this. So they're just cruft. If we > wanted to do something about possible overflows, we'd need to put > assertions here. > > So all in all this is not wrong, but pretty ugly still. Do you want to > change it? If you apply this patch with the above two issues fixed, I'll send another patch to fix the last cast? Is that okay? Fam
On Thu, 08/11 10:46, Fam Zheng wrote: > If you apply this patch with the above two issues fixed, I'll send another > patch to fix the last cast? Is that okay? If we want to apply Corlin's series for dynamic loading, I'll need to rebase anyway. Let's discuss or proceed in his other thread. Fam
diff --git a/block/Makefile.objs b/block/Makefile.objs index 2593a2f..8a8a48e 100644 --- a/block/Makefile.objs +++ b/block/Makefile.objs @@ -1,4 +1,4 @@ -block-obj-y += raw_bsd.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o +block-obj-y += raw_bsd.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o dmg.o block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o block-obj-y += qed-check.o @@ -39,7 +39,7 @@ gluster.o-libs := $(GLUSTERFS_LIBS) ssh.o-cflags := $(LIBSSH2_CFLAGS) ssh.o-libs := $(LIBSSH2_LIBS) archipelago.o-libs := $(ARCHIPELAGO_LIBS) -block-obj-m += dmg.o -dmg.o-libs := $(BZIP2_LIBS) +block-obj-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o +dmg-bz2.o-libs := $(BZIP2_LIBS) qcow.o-libs := -lz linux-aio.o-libs := -laio diff --git a/block/dmg-bz2.c b/block/dmg-bz2.c new file mode 100644 index 0000000..a60e398 --- /dev/null +++ b/block/dmg-bz2.c @@ -0,0 +1,62 @@ +/* + * DMG bzip2 uncompression + * + * Copyright (c) 2004 Johannes E. Schindelin + * Copyright (c) 2016 Red Hat, Int. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +#include "qemu/osdep.h" +#include "qemu-common.h" +#include "dmg.h" +#include <bzlib.h> + +static int dmg_uncompress_bz2_do(char *next_in, unsigned int avail_in, + char *next_out, unsigned int avail_out) +{ + int ret; + uint64_t total_out; + bz_stream bzstream = {}; + + ret = BZ2_bzDecompressInit(&bzstream, 0, 0); + if (ret != BZ_OK) { + return -1; + } + bzstream.next_in = next_in; + bzstream.avail_in = avail_in; + bzstream.next_out = next_out; + bzstream.avail_out = avail_out; + ret = BZ2_bzDecompress(&bzstream); + total_out = ((uint64_t)bzstream.total_out_hi32 << 32) + + bzstream.total_out_lo32; + BZ2_bzDecompressEnd(&bzstream); + if (ret != BZ_STREAM_END || + total_out != avail_out) { + return -1; + } + return 0; +} + +__attribute__((constructor)) +static void dmg_bz2_init(void) +{ + assert(!dmg_uncompress_bz2); + dmg_uncompress_bz2 = dmg_uncompress_bz2_do; +} + diff --git a/block/dmg.c b/block/dmg.c index b0ed89b..861d3aa 100644 --- a/block/dmg.c +++ b/block/dmg.c @@ -28,10 +28,10 @@ #include "qemu/bswap.h" #include "qemu/error-report.h" #include "qemu/module.h" -#include <zlib.h> -#ifdef CONFIG_BZIP2 -#include <bzlib.h> -#endif +#include "dmg.h" + +int (*dmg_uncompress_bz2)(char *next_in, unsigned int avail_in, + char *next_out, unsigned int avail_out); enum { /* Limit chunk sizes to prevent unreasonable amounts of memory being used @@ -41,31 +41,6 @@ enum { DMG_SECTORCOUNTS_MAX = DMG_LENGTHS_MAX / 512, }; -typedef struct BDRVDMGState { - CoMutex lock; - /* each chunk contains a certain number of sectors, - * offsets[i] is the offset in the .dmg file, - * lengths[i] is the length of the compressed chunk, - * sectors[i] is the sector beginning at offsets[i], - * sectorcounts[i] is the number of sectors in that chunk, - * the sectors array is ordered - * 0<=i<n_chunks */ - - uint32_t n_chunks; - uint32_t* types; - uint64_t* offsets; - uint64_t* lengths; - uint64_t* sectors; - uint64_t* sectorcounts; - uint32_t current_chunk; - uint8_t *compressed_chunk; - uint8_t *uncompressed_chunk; - z_stream zstream; -#ifdef CONFIG_BZIP2 - bz_stream bzstream; -#endif -} BDRVDMGState; - static int dmg_probe(const uint8_t *buf, int buf_size, const char *filename) { int len; @@ -210,10 +185,9 @@ static bool dmg_is_known_block_type(uint32_t entry_type) case 0x00000001: /* uncompressed */ case 0x00000002: /* zeroes */ case 0x80000005: /* zlib */ -#ifdef CONFIG_BZIP2 - case 0x80000006: /* bzip2 */ -#endif return true; + case 0x80000006: /* bzip2 */ + return !!dmg_uncompress_bz2; default: return false; } @@ -587,9 +561,6 @@ static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num) if (!is_sector_in_chunk(s, s->current_chunk, sector_num)) { int ret; uint32_t chunk = search_chunk(s, sector_num); -#ifdef CONFIG_BZIP2 - uint64_t total_out; -#endif if (chunk >= s->n_chunks) { return -1; @@ -620,8 +591,10 @@ static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num) return -1; } break; } -#ifdef CONFIG_BZIP2 case 0x80000006: /* bzip2 compressed */ + if (!dmg_uncompress_bz2) { + break; + } /* we need to buffer, because only the chunk as whole can be * inflated. */ ret = bdrv_pread(bs->file, s->offsets[chunk], @@ -630,24 +603,15 @@ static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num) return -1; } - ret = BZ2_bzDecompressInit(&s->bzstream, 0, 0); - if (ret != BZ_OK) { - return -1; - } - s->bzstream.next_in = (char *)s->compressed_chunk; - s->bzstream.avail_in = (unsigned int) s->lengths[chunk]; - s->bzstream.next_out = (char *)s->uncompressed_chunk; - s->bzstream.avail_out = (unsigned int) 512 * s->sectorcounts[chunk]; - ret = BZ2_bzDecompress(&s->bzstream); - total_out = ((uint64_t)s->bzstream.total_out_hi32 << 32) + - s->bzstream.total_out_lo32; - BZ2_bzDecompressEnd(&s->bzstream); - if (ret != BZ_STREAM_END || - total_out != 512 * s->sectorcounts[chunk]) { - return -1; + ret = dmg_uncompress_bz2((char *)s->compressed_chunk, + (unsigned int) s->lengths[chunk], + (char *)s->uncompressed_chunk, + (unsigned int) + 512 * s->sectorcounts[chunk]); + if (ret < 0) { + return ret; } break; -#endif /* CONFIG_BZIP2 */ case 1: /* copy */ ret = bdrv_pread(bs->file, s->offsets[chunk], s->uncompressed_chunk, s->lengths[chunk]); diff --git a/block/dmg.h b/block/dmg.h new file mode 100644 index 0000000..b592d6f --- /dev/null +++ b/block/dmg.h @@ -0,0 +1,59 @@ +/* + * Header for DMG driver + * + * Copyright (c) 2004-2006 Fabrice Bellard + * Copyright (c) 2016 Red hat, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +#ifndef BLOCK_DMG_H +#define BLOCK_DMG_H + +#include "qemu/osdep.h" +#include "qemu-common.h" +#include "block/block_int.h" +#include <zlib.h> + +typedef struct BDRVDMGState { + CoMutex lock; + /* each chunk contains a certain number of sectors, + * offsets[i] is the offset in the .dmg file, + * lengths[i] is the length of the compressed chunk, + * sectors[i] is the sector beginning at offsets[i], + * sectorcounts[i] is the number of sectors in that chunk, + * the sectors array is ordered + * 0<=i<n_chunks */ + + uint32_t n_chunks; + uint32_t *types; + uint64_t *offsets; + uint64_t *lengths; + uint64_t *sectors; + uint64_t *sectorcounts; + uint32_t current_chunk; + uint8_t *compressed_chunk; + uint8_t *uncompressed_chunk; + z_stream zstream; +} BDRVDMGState; + +extern int (*dmg_uncompress_bz2)(char *next_in, unsigned int avail_in, + char *next_out, unsigned int avail_out); + +#endif
dmg.o was moved to block-obj-m in 5505e8b76 to become a separate module, so that its reference to libbz2, since 6b383c08c, doesn't add an extra library to the main executable. We are working on on-demand loading of block drivers which will be easier if all format drivers are always built-in (so that the format probing is not a egg-and-chicken problem). dmg is the only modularized format driver for now. For above reason, we want to move it back to block-obj-y, as is done in this patch. The bz2 uncompressing code that requires bzip2 library is kept in a separate function in the added dmg-bz2.o, which will be loaded at program start in bdrv_init() as usual. It fills in the function pointer inside dmg.o, so that bz2 uncompress can be done in reading path. Signed-off-by: Fam Zheng <famz@redhat.com> --- block/Makefile.objs | 6 ++--- block/dmg-bz2.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++ block/dmg.c | 68 +++++++++++++---------------------------------------- block/dmg.h | 59 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 140 insertions(+), 55 deletions(-) create mode 100644 block/dmg-bz2.c create mode 100644 block/dmg.h