Re: [Qemu-devel] [PATCH 4/7] block/vpc: Improve vpc_open (optimisation and error handling)

[Jeff Cody]

On Fri, Feb 01, 2013 at 10:51:31PM +0100, Stefan Weil wrote:
> * Always read the footer at the end of the image.
>   The footer exists for all kinds of VHD images, so there is no need
>   to read its copy at the start of dynamic VHD images.
> 
> * Return error codes from bdrv_pread like in other block drivers.
> 
> * Return -EMEDIUMTYPE for wrong medium (missing signature).
> 
> * It's not necessary to return 0 on success, >= 0 is sufficient.
> 
> Signed-off-by: Stefan Weil <address@hidden>
> ---
>  block/vpc.c |   46 ++++++++++++++++++++++------------------------
>  1 file changed, 22 insertions(+), 24 deletions(-)
>

Hi Stefan,

I was going to test against some VHD images I have, as well as run
qemu-io tests, but this patch series has conflicts with the current
master.  Patches 1 & 3 have minor conflicts with commit 59294e4, but
this patch has pretty significant conflicts, so I didn't bother
resolving it to test.

Could you rebase, and then repost?

Thanks,
Jeff

> diff --git a/block/vpc.c b/block/vpc.c
> index bcc2ace..fff103b 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -164,34 +164,30 @@ static int vpc_open(BlockDriverState *bs, int flags)
>  {
>      BDRVVPCState *s = bs->opaque;
>      int i;
> -    struct vhd_footer *footer;
> +    struct vhd_footer *footer = (struct vhd_footer *)s->footer_buf;
>      struct vhd_dyndisk_header *dyndisk_header;
>      uint8_t buf[FOOTER_SIZE];
>      uint32_t checksum;
> -    int err = -1;
> +    int ret;
>      int disk_type = VHD_DYNAMIC;
> +    int64_t offset = bdrv_getlength(bs->file);
>  
> -    if (bdrv_pread(bs->file, 0, s->footer_buf, FOOTER_SIZE) != FOOTER_SIZE) {
> +    if (offset < FOOTER_SIZE) {
> +        ret = -EINVAL;
>          goto fail;
>      }
>  
> -    footer = (struct vhd_footer *)s->footer_buf;
> +    /* The footer is always found at the end of the file. */
> +    ret = bdrv_pread(bs->file, offset-FOOTER_SIZE, s->footer_buf, 
> FOOTER_SIZE);
> +    if (ret < 0) {
> +        goto fail;
> +    }
>      if (strncmp(footer->creator, "conectix", 8)) {
> -        int64_t offset = bdrv_getlength(bs->file);
> -        if (offset < FOOTER_SIZE) {
> -            goto fail;
> -        }
> -        /* If a fixed disk, the footer is found only at the end of the file 
> */
> -        if (bdrv_pread(bs->file, offset-FOOTER_SIZE, s->footer_buf, 
> FOOTER_SIZE)
> -                != FOOTER_SIZE) {
> -            goto fail;
> -        }
> -        if (strncmp(footer->creator, "conectix", 8)) {
> -            goto fail;
> -        }
> -        disk_type = VHD_FIXED;
> +        ret = -EMEDIUMTYPE;
> +        goto fail;
>      }
>  
> +    disk_type = be32_to_cpu(footer->type);
>      checksum = be32_to_cpu(footer->checksum);
>      footer->checksum = 0;
>      if (vpc_checksum(s->footer_buf, FOOTER_SIZE) != checksum) {
> @@ -210,19 +206,21 @@ static int vpc_open(BlockDriverState *bs, int flags)
>  
>      /* Allow a maximum disk size of approximately 2 TB */
>      if (bs->total_sectors >= 65535LL * 255 * 255) {
> -        err = -EFBIG;
> +        ret = -EFBIG;
>          goto fail;
>      }
>  
>      if (disk_type == VHD_DYNAMIC) {
> -        if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf,
> -                FOOTER_SIZE) != FOOTER_SIZE) {
> +        ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf,
> +                         FOOTER_SIZE);
> +        if (ret < 0) {
>              goto fail;
>          }
>  
>          dyndisk_header = (struct vhd_dyndisk_header *) buf;
>  
>          if (strncmp(dyndisk_header->magic, "cxsparse", 8)) {
> +            ret = -EINVAL;
>              goto fail;
>          }
>  
> @@ -233,8 +231,9 @@ static int vpc_open(BlockDriverState *bs, int flags)
>          s->pagetable = g_malloc(s->max_table_entries * 4);
>  
>          s->bat_offset = be64_to_cpu(dyndisk_header->table_offset);
> -        if (bdrv_pread(bs->file, s->bat_offset, s->pagetable,
> -                s->max_table_entries * 4) != s->max_table_entries * 4) {
> +        ret = bdrv_pread(bs->file, s->bat_offset, s->pagetable,
> +                         s->max_table_entries * 4);
> +        if (ret < 0) {
>              goto fail;
>          }
>  
> @@ -271,9 +270,8 @@ static int vpc_open(BlockDriverState *bs, int flags)
>                "vpc", bs->device_name, "live migration");
>      migrate_add_blocker(s->migration_blocker);
>  
> -    return 0;
>   fail:
> -    return err;
> +    return ret;
>  }
>  
>  static int vpc_reopen_prepare(BDRVReopenState *state,
> -- 
> 1.7.10.4
>