On Friday 02 January 2015 18:59:38 John Snow wrote:
>
> On 12/27/2014 10:01 AM, Peter Wu wrote:
> > Extract the mish block decoder such that this can be used for other
> > formats in the future. A new DmgHeaderState struct is introduced to
> > share state while decoding.
> >
> > The code is kept unchanged as much as possible, a "fail" label is added
> > for example where a simple return would probably do.
> >
> > Signed-off-by: Peter Wu <pe...@lekensteyn.nl>
> > ---
> > block/dmg.c | 216
> > +++++++++++++++++++++++++++++++++++-------------------------
> > 1 file changed, 125 insertions(+), 91 deletions(-)
> >
> > diff --git a/block/dmg.c b/block/dmg.c
> > index df274f9..6dc6dbb 100644
> > --- a/block/dmg.c
> > +++ b/block/dmg.c
> > @@ -164,19 +164,137 @@ static int64_t dmg_find_koly_offset(BlockDriverState
> > *file_bs)
> > return -EINVAL;
> > }
> >
> > +/* used when building the sector table */
> > +typedef struct DmgHeaderState {
> > + /* used internally by dmg_read_mish_block to remember offsets of blocks
> > + * across calls */
> > + uint64_t last_in_offset;
> > + uint64_t last_out_offset;
> > + /* exported for dmg_open */
> > + uint32_t max_compressed_size;
> > + uint32_t max_sectors_per_chunk;
> > +} DmgHeaderState;
> > +
> > +static int dmg_read_mish_block(BlockDriverState *bs, DmgHeaderState *ds,
> > + int64_t offset, uint32_t count)
> > +{
> > + BDRVDMGState *s = bs->opaque;
> > + uint32_t type, i;
> > + int ret;
> > + size_t new_size;
> > + uint32_t chunk_count;
> > +
> > + ret = read_uint32(bs, offset, &type);
> > + if (ret < 0) {
> > + goto fail;
> > + }
> > +
> > + /* skip data that is not a valid MISH block (invalid magic or too
> > small) */
> > + if (type != 0x6d697368 || count < 244) {
> > + /* assume success for now */
> > + return 0;
> > + }
> > +
> > + offset += 4;
> > + offset += 200;
> > +
> > + chunk_count = (count - 204) / 40;
> > + new_size = sizeof(uint64_t) * (s->n_chunks + chunk_count);
> > + s->types = g_realloc(s->types, new_size / 2);
> > + s->offsets = g_realloc(s->offsets, new_size);
> > + s->lengths = g_realloc(s->lengths, new_size);
> > + s->sectors = g_realloc(s->sectors, new_size);
> > + s->sectorcounts = g_realloc(s->sectorcounts, new_size);
> > +
> > + for (i = s->n_chunks; i < s->n_chunks + chunk_count; i++) {
> > + ret = read_uint32(bs, offset, &s->types[i]);
> > + if (ret < 0) {
> > + goto fail;
> > + }
> > + offset += 4;
> > + if (s->types[i] != 0x80000005 && s->types[i] != 1 &&
> > + s->types[i] != 2) {
> > + if (s->types[i] == 0xffffffff && i > 0) {
> > + ds->last_in_offset = s->offsets[i - 1] + s->lengths[i - 1];
> > + ds->last_out_offset = s->sectors[i - 1] +
> > + s->sectorcounts[i - 1];
> > + }
> > + chunk_count--;
> > + i--;
> > + offset += 36;
> > + continue;
> > + }
> > + offset += 4;
> > +
> > + ret = read_uint64(bs, offset, &s->sectors[i]);
> > + if (ret < 0) {
> > + goto fail;
> > + }
> > + s->sectors[i] += ds->last_out_offset;
> > + offset += 8;
> > +
> > + ret = read_uint64(bs, offset, &s->sectorcounts[i]);
> > + if (ret < 0) {
> > + goto fail;
> > + }
> > + offset += 8;
> > +
> > + if (s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
> > + error_report("sector count %" PRIu64 " for chunk %" PRIu32
> > + " is larger than max (%u)",
> > + s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX);
> > + ret = -EINVAL;
> > + goto fail;
> > + }
> > +
> > + ret = read_uint64(bs, offset, &s->offsets[i]);
> > + if (ret < 0) {
> > + goto fail;
> > + }
> > + s->offsets[i] += ds->last_in_offset;
> > + offset += 8;
> > +
> > + ret = read_uint64(bs, offset, &s->lengths[i]);
> > + if (ret < 0) {
> > + goto fail;
> > + }
> > + offset += 8;
> > +
> > + if (s->lengths[i] > DMG_LENGTHS_MAX) {
> > + error_report("length %" PRIu64 " for chunk %" PRIu32
> > + " is larger than max (%u)",
> > + s->lengths[i], i, DMG_LENGTHS_MAX);
> > + ret = -EINVAL;
> > + goto fail;
> > + }
> > +
> > + update_max_chunk_size(s, i, &ds->max_compressed_size,
> > + &ds->max_sectors_per_chunk);
> > + }
> > + s->n_chunks += chunk_count;
> > + return 0;
> > +
> > +fail:
> > + return ret;
> > +}
> > +
> > static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
> > Error **errp)
> > {
> > BDRVDMGState *s = bs->opaque;
> > - uint64_t info_begin, info_end, last_in_offset, last_out_offset;
> > + DmgHeaderState ds;
> > + uint64_t info_begin, info_end;
> > uint32_t count, tmp;
> > - uint32_t max_compressed_size = 1, max_sectors_per_chunk = 1, i;
> > int64_t offset;
> > int ret;
> >
> > bs->read_only = 1;
> > s->n_chunks = 0;
> > s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
> > + ds.last_in_offset = 0;
> > + ds.last_out_offset = 0;
> > + ds.max_compressed_size = 1;
> > + ds.max_sectors_per_chunk = 1;
>
> It might be nice to have a tiny initializer macro/function so that
> future users of functions that use this structure don't need to know the
> magic numbers.
This state is internal to the dmg_read_mish_block functionality for
which the dmg_open function is the only caller. I consider it similar to
the initialization for BDRVDMGState in the lines above so an additional
indirection to initialize DmgHeaderState does not seem warranted. In the
v2 patch a minor comment is added before this block.
The magic number 1 existed before and seem to be in-place to avoid
allocating a 0 size (for max_sectors_per_chunk). Why max_compressed_size
is 1 is beyond me though, but I copied this value from the previous
version.
> > /* locate the UDIF trailer */
> > offset = dmg_find_koly_offset(bs->file);
> > @@ -210,13 +328,11 @@ static int dmg_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > }
> > info_end = info_begin + count;
> >
> > + /* begin of mish block */
> > offset = info_begin + 0x100;
> >
> > /* read offsets */
> > - last_in_offset = last_out_offset = 0;
> > while (offset < info_end) {
> > - uint32_t type;
> > -
> > ret = read_uint32(bs, offset, &count);
> > if (ret < 0) {
> > goto fail;
> > @@ -226,100 +342,18 @@ static int dmg_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > }
> > offset += 4;
> >
> > - ret = read_uint32(bs, offset, &type);
> > + ret = dmg_read_mish_block(bs, &ds, offset, count);
> > if (ret < 0) {
> > goto fail;
> > }
> > -
> > - if (type == 0x6d697368 && count >= 244) {
> > - size_t new_size;
> > - uint32_t chunk_count;
> > -
> > - offset += 4;
> > - offset += 200;
> > -
> > - chunk_count = (count - 204) / 40;
> > - new_size = sizeof(uint64_t) * (s->n_chunks + chunk_count);
> > - s->types = g_realloc(s->types, new_size / 2);
> > - s->offsets = g_realloc(s->offsets, new_size);
> > - s->lengths = g_realloc(s->lengths, new_size);
> > - s->sectors = g_realloc(s->sectors, new_size);
> > - s->sectorcounts = g_realloc(s->sectorcounts, new_size);
> > -
> > - for (i = s->n_chunks; i < s->n_chunks + chunk_count; i++) {
> > - ret = read_uint32(bs, offset, &s->types[i]);
> > - if (ret < 0) {
> > - goto fail;
> > - }
> > - offset += 4;
> > - if (s->types[i] != 0x80000005 && s->types[i] != 1 &&
> > - s->types[i] != 2) {
> > - if (s->types[i] == 0xffffffff && i > 0) {
> > - last_in_offset = s->offsets[i - 1] + s->lengths[i
> > - 1];
> > - last_out_offset = s->sectors[i - 1] +
> > - s->sectorcounts[i - 1];
> > - }
> > - chunk_count--;
> > - i--;
> > - offset += 36;
> > - continue;
> > - }
> > - offset += 4;
> > -
> > - ret = read_uint64(bs, offset, &s->sectors[i]);
> > - if (ret < 0) {
> > - goto fail;
> > - }
> > - s->sectors[i] += last_out_offset;
> > - offset += 8;
> > -
> > - ret = read_uint64(bs, offset, &s->sectorcounts[i]);
> > - if (ret < 0) {
> > - goto fail;
> > - }
> > - offset += 8;
> > -
> > - if (s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
> > - error_report("sector count %" PRIu64 " for chunk %"
> > PRIu32
> > - " is larger than max (%u)",
> > - s->sectorcounts[i], i,
> > DMG_SECTORCOUNTS_MAX);
> > - ret = -EINVAL;
> > - goto fail;
> > - }
> > -
> > - ret = read_uint64(bs, offset, &s->offsets[i]);
> > - if (ret < 0) {
> > - goto fail;
> > - }
> > - s->offsets[i] += last_in_offset;
> > - offset += 8;
> > -
> > - ret = read_uint64(bs, offset, &s->lengths[i]);
> > - if (ret < 0) {
> > - goto fail;
> > - }
> > - offset += 8;
> > -
> > - if (s->lengths[i] > DMG_LENGTHS_MAX) {
> > - error_report("length %" PRIu64 " for chunk %" PRIu32
> > - " is larger than max (%u)",
> > - s->lengths[i], i, DMG_LENGTHS_MAX);
> > - ret = -EINVAL;
> > - goto fail;
> > - }
> > -
> > - update_max_chunk_size(s, i, &max_compressed_size,
> > - &max_sectors_per_chunk);
> > - }
> > - s->n_chunks += chunk_count;
> > - }
> > + offset += count;
>
> A little justification here might be nice: I assume the "count" always
> includes the full byte count of the mish block, and the old code just
> incremented the offset up to this amount. This is the only functional
> difference in this refactor that I can see.
This code got me confused again and after looking at this document[1], I
have added some explanatory comments for the next version.
The "count" in the loop refers to the contents of a single resource.
Basically we have this:
Resource header
- 4 bytes rsrc data offset (assume 0x100)
- 4 bytes rsrc map offset (ignored)
- 4 bytes rsrc data length
- 4 bytes rsrc map length (ignored)
The block of length "rsrc data length" at offset 0x100 consists of one
or more resources, each described by:
- 4 bytes resource length
- "resource length" number of bytes. Its contents is a "mish" block for
our purposes.
Indeed, in this patch I avoided the need to copy from the mish block
parser as it is not really necessary given that the resource length is
known.
[1]:
https://developer.apple.com/legacy/library/documentation/mac/pdf/MoreMacintoshToolbox.pdf#page=151
> > }
> >
> > /* initialize zlib engine */
> > s->compressed_chunk = qemu_try_blockalign(bs->file,
> > - max_compressed_size + 1);
> > + ds.max_compressed_size + 1);
> > s->uncompressed_chunk = qemu_try_blockalign(bs->file,
> > - 512 *
> > max_sectors_per_chunk);
> > + 512 *
> > ds.max_sectors_per_chunk);
> > if (s->compressed_chunk == NULL || s->uncompressed_chunk == NULL) {
> > ret = -ENOMEM;
> > goto fail;
> >
>
> Thank you for splitting this monolithic function up into nicer pieces.
> With or without addressing my comments above:
>
> Reviewed-by: John Snow <js...@redhat.com>
Since I have only added some comments and renamed a variable, I'll carry
this R-b to the v2 patch.
--
Kind regards,
Peter
https://lekensteyn.nl
