Re: [Qemu-devel] [PATCH v2 01/12] block/dmg: properly detect the UDIF trailer

[Stefan Hajnoczi]

On Wed, Jan 07, 2015 at 03:19:13PM +0100, Peter Wu wrote:
> On Wednesday 07 January 2015 13:19:34 Stefan Hajnoczi wrote:
> > On Tue, Jan 06, 2015 at 06:48:04PM +0100, Peter Wu wrote:
> > > DMG files have a variable length with a UDIF trailer at the end of a
> > > file. This UDIF trailer is essential as it describes the contents of
> > > the image. At the moment however, the start of this trailer is almost
> > > always incorrect as bdrv_getlength() returns a multiple of the block
> > > size (rounded up). This results in a failure to recognize DMG files,
> > > resulting in Invalid argument (EINVAL) errors.
> > > 
> > > As there is no API to retrieve the real file size, look for the magic
> > > header in the last two sectors to find the start of this 512-byte UDIF
> > > trailer (the "koly" block).
> > > 
> > > The resource fork offset ("info_begin") has its offset adjusted as the
> > > initial value of offset does not mean "end of file" anymore, but "begin
> > > of UDIF trailer".
> > > 
> > > Signed-off-by: Peter Wu <address@hidden>
> > > Reviewed-by: John Snow <address@hidden>
> > > ---
> > >  v2: added R-b, set errp as suggested by Stefan Hajnoczi
> > > ---
> > >  block/dmg.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
> > >  1 file changed, 45 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/block/dmg.c b/block/dmg.c
> > > index e455886..9183459 100644
> > > --- a/block/dmg.c
> > > +++ b/block/dmg.c
> > > @@ -131,6 +131,48 @@ static void update_max_chunk_size(BDRVDMGState *s, 
> > > uint32_t chunk,
> > >      }
> > >  }
> > >  
> > > +static int64_t dmg_find_koly_offset(BlockDriverState *file_bs, Error 
> > > **errp)
> > > +{
> > > +    int64_t length;
> > > +    int64_t offset = 0;
> > > +    uint8_t buffer[515];
> > > +    int i, ret;
> > > +
> > > +    /* bdrv_getlength returns a multiple of block size (512), rounded 
> > > up. Since
> > > +     * dmg images can have odd sizes, try to look for the "koly" magic 
> > > which
> > > +     * marks the begin of the UDIF trailer (512 bytes). This magic can 
> > > be found
> > > +     * in the last 511 bytes of the second-last sector or the first 4 
> > > bytes of
> > > +     * the last sector (search space: 515 bytes) */
> > > +    length = bdrv_getlength(file_bs);
> > > +    if (length < 0) {
> > > +        error_setg_errno(errp, -length,
> > > +            "Failed to get file size while reading UDIF trailer");
> > > +        return length;
> > > +    } else if (length < 512) {
> > > +        error_set(errp, ERROR_CLASS_GENERIC_ERROR,
> > > +            "dmg file must be at least 512 bytes long");
> > > +        return -EINVAL;
> > 
> > Not worth respinning but error_setg() is a shortcut for error_set(errp,
> > ERROR_CLASS_GENERIC_ERROR, fmt, ...).
> > 
> > Reviewed-by: Stefan Hajnoczi <address@hidden>
> 
> Good to know, I got a compile error when ERROR_CLASS_GENERIC_ERROR was
> omitted, didn't think of using error_setg instead.
> 
> When merging, please replace the above ERROR_CLASS_GENERIC_ERROR by:
> 
>         error_setg(errp, "dmg file must be at least 512 bytes long");
> 
> (and likewise for the last error_set in the function)

Done!

Stefan

pgp7j31vK_Hp7.pgp
Description: PGP signature