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

[Max Reitz]

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 <address@hidden>
> ---
>  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]);

signature.asc
Description: OpenPGP digital signature