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

[Kevin Wolf]

Am 25.01.2013 14:37, schrieb Markus Armbruster:
> Kevin Wolf <address@hidden> writes:
> 
>> Am 24.01.2013 14:09, schrieb 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;
>                  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;
>              }
>>>
>>> What if 0 <= bdrv_getlength() < HEADER_SIZE?
>>
>> Then offset - HEADER_SIZE becomes negative. Not sure what this means, I
>> need to check [...]
>> I'm strongly for leaving the check in for now.
> 
> Sounds like you're thinking about what happens in the bdrv_pread() a few
> lines down.  Not reached, because we goto fail, and return a
> non-negative number.  That's what worries me.

Oh, I completely missed that. I thought you were asking what happened if
we dropped the check like you suggested.

You're right, this needs separate checks for < 0 and < HEADER_SIZE, the
latter probably returning -EINVAL.

Kevin