Re: [Qemu-devel] [PATCH 3/6] vpc: Fix bdrv_open() error handling

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Return -errno instead of -1 on errors. While touching the
> code, fix a memory leak.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block/vpc.c |   36 +++++++++++++++++++++++++-----------
>  1 files changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/block/vpc.c b/block/vpc.c
> index 7948609..9d2b177 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -163,24 +163,29 @@ static int vpc_open(BlockDriverState *bs, int flags)
>      struct vhd_dyndisk_header* dyndisk_header;
>      uint8_t buf[HEADER_SIZE];
>      uint32_t checksum;
> -    int err = -1;
>      int disk_type = VHD_DYNAMIC;
> +    int ret;
>  
> -    if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE)
> +    ret = bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE);
> +    if (ret < 0 ) {
>          goto fail;
> +    }
>  
>      footer = (struct vhd_footer*) s->footer_buf;
>      if (strncmp(footer->creator, "conectix", 8)) {
>          int64_t offset = bdrv_getlength(bs->file);
>          if (offset < HEADER_SIZE) {
> +            ret = offset;

What if 0 <= bdrv_getlength() < HEADER_SIZE?

For what it's worth, in other places, we simply bdrv_read() without
checking "got enough" first.  As far as I can tell, bdrv_read() returns
-EIO when it hits EOF prematurely.

>              goto fail;
>          }
>          /* If a fixed disk, the footer is found only at the end of the file 
> */
> -        if (bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf, 
> HEADER_SIZE)
> -                != HEADER_SIZE) {
> +        ret = bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf,
> +                         HEADER_SIZE);
> +        if (ret < 0) {
>              goto fail;
>          }
>          if (strncmp(footer->creator, "conectix", 8)) {
> +            ret = -EMEDIUMTYPE;

I figure this makes your series depends on Stefan Weil's "block: Fix
error report for wrong file format".  I'd probably order it the other
way, and return -EINVAL here, then change it along with the others in
"block: Use error code EMEDIUMTYPE for wrong format in some block
drivers".  Matter of taste.

>              goto fail;
>          }
>          disk_type = VHD_FIXED;
       }

       checksum = be32_to_cpu(footer->checksum);
       footer->checksum = 0;
       if (vpc_checksum(s->footer_buf, HEADER_SIZE) != checksum)
           fprintf(stderr, "block-vpc: The header checksum of '%s' is "
               "incorrect.\n", bs->filename);

I wonder whether this should fail the open.  Outside the scope of this
patch.

> @@ -203,19 +208,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,
> -                HEADER_SIZE) != HEADER_SIZE) {
> +        ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf,
> +                         HEADER_SIZE);
> +        if (ret < 0) {
>              goto fail;
>          }
>  
>          dyndisk_header = (struct vhd_dyndisk_header *) buf;
>  
>          if (strncmp(dyndisk_header->magic, "cxsparse", 8)) {
> +            ret = -EINVAL;

If you keep -EMEDIUMTYPE above, should this be -EMEDIUMTYPE, too?

>              goto fail;
>          }
>  
> @@ -226,8 +233,10 @@ 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;
>          }
>  
> @@ -265,8 +274,13 @@ static int vpc_open(BlockDriverState *bs, int flags)
>      migrate_add_blocker(s->migration_blocker);
>  
>      return 0;
> - fail:
> -    return err;
> +
> +fail:
> +    g_free(s->pagetable);
> +#ifdef CACHE
> +    g_free(s->pageentry_u8);
> +#endif
> +    return ret;
>  }
>  
>  static int vpc_reopen_prepare(BDRVReopenState *state,