Re: [Qemu-devel] [PATCH 00/10] block/dmg: (compatibility) fixes and bzip2 support

[Peter Wu <peter@lekensteyn.nl>]

On Friday 02 January 2015 13:58:33 John Snow wrote:
> 
> On 01/02/2015 01:46 PM, Peter Wu wrote:
> > FYI, I plan to make some more changes:
> >
> > - do not require offset ≠ 0 for resource fork and XML offsets.
> > Technically it is allowed, do you agree on this change?
> 
> If you have seen this in the wild, I definitely agree. If you haven't, I 
> am not against the change, but there's likely no hurry to include it in 
> this series if the changes are not simple.

It would involve only a removal of "rsrc_fork_offset != 0 && " in patch
3 and "plist_xml_offset != 0 && " in patch 5. I have not seen it in the
real world, only when trying to construct a dmg file by hand for testing
purposes. The change is simple and can be squashed in the patch.

It makes sense since previously only the offset was checked. Now the
length is checked instead. Before:

    /* read offset */
    ret = read_uint64(bs, offset, &info_begin);
    if (ret < 0) {
        goto fail;
    } else if (info_begin == 0) {
        /* assume invalid file when offset is zero */
        ret = -EINVAL;
        goto fail;
    } 

After (in current patch series):

    /* offset of resource fork (RsrcForkOffset) */
    ret = read_uint64(bs, offset + 0x28, &rsrc_fork_offset);
    if (ret < 0) {
        goto fail;
    }
    ret = read_uint64(bs, offset + 0x30, &rsrc_fork_length);
    if (ret < 0) {
        goto fail;
    }
    // ...
    if (rsrc_fork_offset != 0 && rsrc_fork_length != 0) {
        ret = dmg_read_resource_fork(bs, &ds,
    

In the current patch series both the offset and lengths are checked, but
it is sufficient to look at just the length.

Kind regards,
Peter

> > - improve offset checking
> > https://git.lekensteyn.nl/peter/qemu/commit/?h=block-dmg-2.3&id=41fd83773361923f668f54796ff563660b77e96c
> > (squash with the existing length checking patch)
> >
> > - (not part of this series, but for future consideration) read
> > errors currently return 1 (EPERM). EIO or EINVAL would probably a
> > better choice depending on the error type.
> >
> > Other than that, the patches should be ready for review. Thank you
> > in advance.
> >
> > Kind regards,
> > Peter
> > https://lekensteyn.nl
> > (pardon my brevity, top-posting and formatting, sent from my phone)
> >
> >
> > On January 2, 2015 5:31:33 PM CET, John Snow <jsnow@redhat.com> wrote:
> >>
> >>
> >> On 01/02/2015 09:14 AM, Stefan Hajnoczi wrote:
> >>> On Sat, Dec 27, 2014 at 04:01:34PM +0100, Peter Wu wrote:
> >>>> These series improve QEMU support for DMG image files:
> >>>
> >>> Hi,
> >>> Thanks for this patch series.  Kevin and I consider patches for
> >> merging
> >>> after they have a Reviewed-by: from at least 1 other QEMU
> >> contributor.
> >>>
> >>> I have CCed John Snow.
> >>>
> >>> John: If you are busy, please CC someone else or let us know so this
> >>> series can get reviewed.
> >>>
> >>> Stefan
> >>>
> >>
> >> Just recomposing myself post-vacation, I will start looking this over
> >> today.
> >>
> >> --John