Re: [Qemu-devel] [PATCH] block/vmdk: add basic .bdrv_check support

Re: [Qemu-devel] [PATCH] block/vmdk: add basic .bdrv_check support

[Fam Zheng]

On Mon, 01/13 12:24, Peter Lieven wrote:
> this adds a basic vmdk corruption check. it should detect severe
> table corruptions and file truncation.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/vmdk.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index c6b60b4..1d66858 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1918,6 +1918,51 @@ static ImageInfo *vmdk_get_extent_info(VmdkExtent *extent)
>      return info;
>  }
>  
> +static int vmdk_check(BlockDriverState *bs, BdrvCheckResult *result,
> +                      BdrvCheckMode fix)
> +{
> +    BDRVVmdkState *s = bs->opaque;
> +    VmdkExtent *extent = NULL;
> +    int64_t sector_num = 0;
> +    int64_t total_sectors = bdrv_getlength(bs) / BDRV_SECTOR_SIZE;
> +    int ret;
> +    uint64_t cluster_offset;
> +
> +    if (fix) {
> +        return -ENOTSUP;
> +    }
> +
> +    for (;;) {
> +        if (sector_num >= total_sectors) {
> +            return 0;
> +        }
> +        extent = find_extent(s, sector_num, extent);
> +        if (!extent) {
> +            fprintf(stderr, "ERROR: could not find extend for sector %ld\n",
> +                    sector_num);
> +            break;
> +        }
> +        ret = get_cluster_offset(bs, extent, NULL, sector_num << BDRV_SECTOR_BITS,
> +                                 0, &cluster_offset);
> +        if (ret == VMDK_ERROR) {
> +            fprintf(stderr,
> +                    "ERROR: could not get cluster_offset for sector %ld\n",
> +                    sector_num);
> +            break;
> +        }
> +        if (ret == VMDK_OK && cluster_offset >= bdrv_getlength(extent->file)) {
> +            fprintf(stderr,
> +                    "ERROR: cluster offset for sector %ld points after EOF\n",
> +                    sector_num);
> +            break;
> +        }
> +        sector_num += extent->cluster_sectors;
> +    }
> +
> +    result->corruptions++;
> +    return 0;
> +}
> +
>  static ImageInfoSpecific *vmdk_get_specific_info(BlockDriverState *bs)
>  {
>      int i;
> @@ -1991,6 +2036,7 @@ static BlockDriver bdrv_vmdk = {
>      .instance_size                = sizeof(BDRVVmdkState),
>      .bdrv_probe                   = vmdk_probe,
>      .bdrv_open                    = vmdk_open,
> +    .bdrv_check                   = vmdk_check,
>      .bdrv_reopen_prepare          = vmdk_reopen_prepare,
>      .bdrv_read                    = vmdk_co_read,
>      .bdrv_write                   = vmdk_co_write,

Works for me. Thank.

And BTW, is it worth comparing with the "grain_offset" header field? This
should be the first data cluster's offset. When you have a fresh image with no
data cluster allocated, and it's truncated, this patch doesn't catch it. Anyway,

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH] block/vmdk: add basic .bdrv_check support

[Stefan Hajnoczi]

On Mon, Jan 13, 2014 at 12:24:06PM +0100, Peter Lieven wrote:
> +    for (;;) {
> +        if (sector_num >= total_sectors) {
> +            return 0;
> +        }
> +        extent = find_extent(s, sector_num, extent);
> +        if (!extent) {
> +            fprintf(stderr, "ERROR: could not find extend for sector %ld\n",
> +                    sector_num);

s/extend/extent/

Please use PRId64 instead of %ld so this works on 32-bit hosts.

> +            break;
> +        }
> +        ret = get_cluster_offset(bs, extent, NULL, sector_num << BDRV_SECTOR_BITS,
> +                                 0, &cluster_offset);
> +        if (ret == VMDK_ERROR) {
> +            fprintf(stderr,
> +                    "ERROR: could not get cluster_offset for sector %ld\n",

PRId64

> +                    sector_num);
> +            break;
> +        }
> +        if (ret == VMDK_OK && cluster_offset >= bdrv_getlength(extent->file)) {
> +            fprintf(stderr,
> +                    "ERROR: cluster offset for sector %ld points after EOF\n",

PRId64

Re: [Qemu-devel] [PATCH] block/vmdk: add basic .bdrv_check support

[Peter Lieven]

Am 27.01.2014 16:37, schrieb Stefan Hajnoczi:
> On Mon, Jan 13, 2014 at 12:24:06PM +0100, Peter Lieven wrote:
>> +    for (;;) {
>> +        if (sector_num >= total_sectors) {
>> +            return 0;
>> +        }
>> +        extent = find_extent(s, sector_num, extent);
>> +        if (!extent) {
>> +            fprintf(stderr, "ERROR: could not find extend for sector %ld\n",
>> +                    sector_num);
> s/extend/extent/
>
> Please use PRId64 instead of %ld so this works on 32-bit hosts.
>
>> +            break;
>> +        }
>> +        ret = get_cluster_offset(bs, extent, NULL, sector_num << BDRV_SECTOR_BITS,
>> +                                 0, &cluster_offset);
>> +        if (ret == VMDK_ERROR) {
>> +            fprintf(stderr,
>> +                    "ERROR: could not get cluster_offset for sector %ld\n",
> PRId64
>
>> +                    sector_num);
>> +            break;
>> +        }
>> +        if (ret == VMDK_OK && cluster_offset >= bdrv_getlength(extent->file)) {
>> +            fprintf(stderr,
>> +                    "ERROR: cluster offset for sector %ld points after EOF\n",
> PRId64
will send a respin.

Peter