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 <f...@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
