Re: [Qemu-devel] [PATCH for-2.8] dmg: Move the driver to block-obj-y

[Fam Zheng <famz@redhat.com>]

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