Am 25.01.2013 14:37, schrieb Markus Armbruster:
> Kevin Wolf <kw...@redhat.com> writes:
>
>> Am 24.01.2013 14:09, schrieb Markus Armbruster:
>>> 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;
> 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
