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;
>
> 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.
> 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.
> 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.
I think there was a reason why not, but I can't remember the details.
>> @@ -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?
Good question, I wondered the same when writing the patch.
I decided to stay consistent with vpc_probe() which detects an image as
VHD image if the first magic is right. Then this case means that it's
still a VHD image, but an invalid one.
Kevin
