[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/6] cloop: Fix bdrv_open() error handling
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 2/6] cloop: Fix bdrv_open() error handling |
Date: |
Thu, 24 Jan 2013 14:17:57 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120605 Thunderbird/13.0 |
Am 24.01.2013 13:48, 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/cloop.c | 27 +++++++++++++++++----------
>> 1 files changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/cloop.c b/block/cloop.c
>> index 5a0d0d8..9b36063 100644
>> --- a/block/cloop.c
>> +++ b/block/cloop.c
>> @@ -57,27 +57,32 @@ static int cloop_open(BlockDriverState *bs, int flags)
>> {
>> BDRVCloopState *s = bs->opaque;
>> uint32_t offsets_size, max_compressed_block_size = 1, i;
>> + int ret;
>>
>> bs->read_only = 1;
>>
>> /* read header */
>> - if (bdrv_pread(bs->file, 128, &s->block_size, 4) < 4) {
>> - goto cloop_close;
>> + ret = bdrv_pread(bs->file, 128, &s->block_size, 4);
>> + if (ret < 0) {
>> + return ret;
>> }
>> s->block_size = be32_to_cpu(s->block_size);
>>
>> - if (bdrv_pread(bs->file, 128 + 4, &s->n_blocks, 4) < 4) {
>> - goto cloop_close;
>> + ret = bdrv_pread(bs->file, 128 + 4, &s->n_blocks, 4);
>> + if (ret < 0) {
>> + return ret;
>> }
>> s->n_blocks = be32_to_cpu(s->n_blocks);
>>
>> /* read offsets */
>> offsets_size = s->n_blocks * sizeof(uint64_t);
>> s->offsets = g_malloc(offsets_size);
>> - if (bdrv_pread(bs->file, 128 + 4 + 4, s->offsets, offsets_size) <
>> - offsets_size) {
>> - goto cloop_close;
>> +
>
> Empty line visually detaches the /* read offsets */ comment from the
> actual read. Sure you want it?
For comments like this, which organise the code in sections, I generally
don't assume that a comment is only valid until the next empty line, but
until the next comment comes.
I like the empty line here, but if you don't, I can remove it.
>> + ret = bdrv_pread(bs->file, 128 + 4 + 4, s->offsets, offsets_size);
>> + if (ret < 0) {
>> + goto fail;
>> }
>> +
>> for(i=0;i<s->n_blocks;i++) {
>> s->offsets[i] = be64_to_cpu(s->offsets[i]);
>> if (i > 0) {
>> @@ -92,7 +97,8 @@ static int cloop_open(BlockDriverState *bs, int flags)
>> s->compressed_block = g_malloc(max_compressed_block_size + 1);
>> s->uncompressed_block = g_malloc(s->block_size);
>> if (inflateInit(&s->zstream) != Z_OK) {
>> - goto cloop_close;
>> + ret = -EINVAL;
>
> inflateInit() can return a number of different errors. But your change
> doesn't make things worse, and that's good enough.
It doesn't use errno and I decided that I don't care enough about these
block drivers to write an error conversion function... As you said, it
doesn't make things worse, and it's also not the most likely error to
happen.
>> + goto fail;
>> }
>> s->current_block = s->n_blocks;
>>
>> @@ -101,8 +107,9 @@ static int cloop_open(BlockDriverState *bs, int flags)
>> qemu_co_mutex_init(&s->lock);
>> return 0;
>>
>> -cloop_close:
>> - return -1;
>> +fail:
>> + g_free(s->offsets);
>
> What about s->compressed_block and s->uncompressed_block?
Thanks, I missed them. Will fix.
Kevin
[Qemu-devel] [PATCH 5/6] dmg: Use g_free instead of free, Kevin Wolf, 2013/01/24