Re: [Qemu-devel] [PATCH v2 09/12] block/dmg: use SectorNumber from BLKX header

[John Snow <jsnow@redhat.com>]

On 01/06/2015 12:48 PM, Peter Wu wrote:
> Previously the sector table parsing relied on the previous offset of
> the DMG file. Now it uses the sector number from the BLKX header
> (see http://newosxbook.com/DMG.html).
>
> The implementation of dmg2img (from vu1tur) does not base the output
> sector on the location of the terminator (0xffffffff) either so it
> should be safe to drop this dependency on the previous state.
>
> (It makes somehow makes sense, a terminator should halt further
> processing of a block and is perhaps used to preallocate some space.)
>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
>   v2: initial patch after suggestions from John Snow to read and use
>       these fields.
> ---
>   block/dmg.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/block/dmg.c b/block/dmg.c
> index 130efac..57922c5 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -189,7 +189,6 @@ typedef struct DmgHeaderState {
>       /* used internally by dmg_read_mish_block to remember offsets of blocks
>        * across calls */
>       uint64_t data_fork_offset;
> -    uint64_t last_out_offset;
>       /* exported for dmg_open */
>       uint32_t max_compressed_size;
>       uint32_t max_sectors_per_chunk;
> @@ -205,6 +204,7 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>       int64_t offset = 0;
>       uint64_t data_offset;
>       uint64_t in_offset = ds->data_fork_offset;
> +    uint64_t out_offset;
>
>       type = buff_read_uint32(buffer, offset);
>       /* skip data that is not a valid MISH block (invalid magic or too small) */
> @@ -213,6 +213,9 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>           return 0;
>       }
>
> +    /* chunk offsets are relative to this sector number */
> +    out_offset = buff_read_uint64(buffer, offset + 8);
> +
>       /* location in data fork for (compressed) blob (in bytes) */
>       data_offset = buff_read_uint64(buffer, offset + 0x18);
>       in_offset += data_offset;
> @@ -233,10 +236,6 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>           offset += 4;
>           if (s->types[i] != 0x80000005 && s->types[i] != 1 &&
>               s->types[i] != 2) {
> -            if (s->types[i] == 0xffffffff && i > 0) {
> -                ds->last_out_offset = s->sectors[i - 1] +
> -                                      s->sectorcounts[i - 1];
> -            }

This is exactly what I was hoping to see could be eliminated by reading 
the mish headers. Thanks for this.

>               chunk_count--;
>               i--;
>               offset += 36;
> @@ -245,7 +244,7 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>           offset += 4;
>
>           s->sectors[i] = buff_read_uint64(buffer, offset);
> -        s->sectors[i] += ds->last_out_offset;
> +        s->sectors[i] += out_offset;
>           offset += 8;
>
>           s->sectorcounts[i] = buff_read_uint64(buffer, offset);
> @@ -419,7 +418,6 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
>       s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
>       /* used by dmg_read_mish_block to keep track of the current I/O position */
>       ds.data_fork_offset = 0;
> -    ds.last_out_offset = 0;
>       ds.max_compressed_size = 1;
>       ds.max_sectors_per_chunk = 1;
>
>

Much nicer.

Reviewed-by: John Snow <jsnow@redhat.com>