Kevin Wolf <kw...@redhat.com> writes:
> Return -errno instead of -1 on errors. While touching the
> code, fix a memory leak.
>
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
> 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,
