Am 24.01.2013 13:48, 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/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