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.
>
> offset is signed and the offset parameter of bdrv_pread() is int64_t as
> well. In there, sector_num will become negative as well and is passed as
> int64_t to bdrv_read, bdrv_rw_co, bdrv_rw_co_entry, bdrv_co_do_readv,
> bdrv_check_request. It then is multiplied by BDRV_SECTOR_SIZE and passed
> to bdrv_check_byte_request.
>
> bs->file->growable = 1 because it is opened with bdrv_file_open(),
> therefore bdrv_check_byte_request doesn't complain.
>
> The negative sector number is then passed to the block driver, which can
> do with it whatever it likes. Just some examples:
>
> * raw-posix with aio=threads will store it in RawPosixAIOData.aio_offset
> and later pass it to preadv/pread. This should return -EINVAL.
>
> * raw-win32 stores it in Offset and OffsetHigh in an OVERLAPPED struct,
> which both seem to be a DWORD. If I should guess, that's a uint32_t,
> so we get a position far after EOF. No idea what happens, maybe we're
> lucky and ReadFile() errors out.
>
> * nbd places it in struct nbd_request.from, which is uint64_t. Pretty
> large request, but hopefully the server will do something reasonable
> with it.
>
> So I wouldn't bet that bdrv_pread() handles negative offset correctly
> (let alone consistently) in all cases. We should probably fix this, but
> certainly not in this patch.
>
> 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.
>> 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.
>
> You're thinking of a different case here. But now that you brought it
> up, let me mention that bs->growable means that at least raw-posix reads
> zeros after EOF instead of failing.
Aha. Didn't realize that growable applies to reads as well.
[...]
