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

[Stefan Hajnoczi]

On Sat, Dec 27, 2014 at 04:01:35PM +0100, Peter Wu wrote:
> diff --git a/block/dmg.c b/block/dmg.c
> index e455886..df274f9 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -131,6 +131,39 @@ static void update_max_chunk_size(BDRVDMGState *s, 
> uint32_t chunk,
>      }
>  }
>  
> +static int64_t dmg_find_koly_offset(BlockDriverState *file_bs)
> +{
> +    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 < 512) {
> +        return length < 0 ? length : -EINVAL;

dmg_open() should pass in Error *errp so a detailed error reporting can
be used:

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, "dmg file must be at least 512 bytes long");
    return -EINVAL;
}

This makes it much easier to pinpoint errors (instead of just -EINVAL)
and also gives the user a hint about the cause.

> +    }
> +    if (length > 511 + 512) {
> +        offset = length - 511 - 512;
> +    }
> +    length = length < 515 ? length : 515;
> +    ret = bdrv_pread(file_bs, offset, buffer, length);
> +    if (ret < 4) {
> +        return ret < 0 ? ret : -EINVAL;

bdrv_pread() does not return short reads.  The return value will either
be length or an error.  This could be just:

if (ret < 0) {
    error_setg_errno(errp, -ret, "Failed to read last sectors in dmg file");
    return ret;
}

(The unique error string makes it easy to track down the location where
the error occurs.)

> +    }
> +    for (i = 0; i < length - 3; i++) {
> +        if (buffer[i] == 'k' && buffer[i+1] == 'o' &&
> +            buffer[i+2] == 'l' && buffer[i+3] == 'y') {
> +            return offset + i;
> +        }
> +    }
> +    return -EINVAL;

error_set(errp, "Not a dmg file, unable to find UDIF footer");
return -EINVAL;

pgplHJG6gHeF_.pgp
Description: PGP signature