[4/7] block/vpc: Improve vpc_open (optimisation and error handling)

* 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 <sw@weilnetz.de>
---
 block/vpc.c |   46 ++++++++++++++++++++++------------------------
 1 file changed, 22 insertions(+), 24 deletions(-)

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 <sw@weilnetz.de>
> ---
>  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
>

Am 01.02.2013 22:51, schrieb Stefan Weil:
> * 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 <sw@weilnetz.de>
> ---
>  block/vpc.c |   46 ++++++++++++++++++++++------------------------
>  1 file changed, 22 insertions(+), 24 deletions(-)

> @@ -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;
>  }

I'd rather leave the return 0; there, it's more obvious than reusing a
return value from "somewhere", of which we just happen know that it
contains a non-negative number if we did the previous error handling right.

Kevin