| Message ID | 20170829120836.16091-1-berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series | misc: Remove unused Error variables | expand |
Hi Alberto, On 08/29/2017 09:08 AM, Alberto Garcia wrote: > There's a few cases which we're passing an Error pointer to a function > only to discard it immediately afterwards without checking it. In > these cases we can simply remove the variable and pass NULL instead. How did you notice? > > Signed-off-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > block/qcow.c | 12 +++--------- > block/qcow2.c | 8 ++------ > dump.c | 4 +--- > 3 files changed, 6 insertions(+), 18 deletions(-) > > diff --git a/block/qcow.c b/block/qcow.c > index c08cdc4a7b..63904a26ee 100644 > --- a/block/qcow.c > +++ b/block/qcow.c > @@ -454,13 +454,11 @@ static uint64_t get_cluster_offset(BlockDriverState *bs, > start_sect = (offset & ~(s->cluster_size - 1)) >> 9; > for(i = 0; i < s->cluster_sectors; i++) { > if (i < n_start || i >= n_end) { > - Error *err = NULL; > memset(s->cluster_data, 0x00, 512); > if (qcrypto_block_encrypt(s->crypto, start_sect + i, > s->cluster_data, > BDRV_SECTOR_SIZE, > - &err) < 0) { > - error_free(err); > + NULL) < 0) { > errno = EIO; > return -1; > } > @@ -572,7 +570,6 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num, > QEMUIOVector hd_qiov; > uint8_t *buf; > void *orig_buf; > - Error *err = NULL; > > if (qiov->niov > 1) { > buf = orig_buf = qemu_try_blockalign(bs, qiov->size); > @@ -637,7 +634,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num, > if (bs->encrypted) { > assert(s->crypto); > if (qcrypto_block_decrypt(s->crypto, sector_num, buf, > - n * BDRV_SECTOR_SIZE, &err) < 0) { > + n * BDRV_SECTOR_SIZE, NULL) < 0) { > goto fail; > } > } > @@ -660,7 +657,6 @@ done: > return ret; > > fail: > - error_free(err); > ret = -EIO; > goto done; > } > @@ -709,11 +705,9 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num, > break; > } > if (bs->encrypted) { > - Error *err = NULL; > assert(s->crypto); > if (qcrypto_block_encrypt(s->crypto, sector_num, buf, > - n * BDRV_SECTOR_SIZE, &err) < 0) { > - error_free(err); > + n * BDRV_SECTOR_SIZE, NULL) < 0) { > ret = -EIO; > break; > } > diff --git a/block/qcow2.c b/block/qcow2.c > index 40ba26c111..fbfffadc76 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -1820,15 +1820,13 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset, > assert(s->crypto); > assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0); > assert((cur_bytes & (BDRV_SECTOR_SIZE - 1)) == 0); > - Error *err = NULL; > if (qcrypto_block_decrypt(s->crypto, > (s->crypt_physical_offset ? > cluster_offset + offset_in_cluster : > offset) >> BDRV_SECTOR_BITS, > cluster_data, > cur_bytes, > - &err) < 0) { > - error_free(err); > + NULL) < 0) { > ret = -EIO; > goto fail; > } > @@ -1942,7 +1940,6 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset, > qemu_iovec_concat(&hd_qiov, qiov, bytes_done, cur_bytes); > > if (bs->encrypted) { > - Error *err = NULL; > assert(s->crypto); > if (!cluster_data) { > cluster_data = qemu_try_blockalign(bs->file->bs, > @@ -1963,8 +1960,7 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset, > cluster_offset + offset_in_cluster : > offset) >> BDRV_SECTOR_BITS, > cluster_data, > - cur_bytes, &err) < 0) { > - error_free(err); > + cur_bytes, NULL) < 0) { > ret = -EIO; > goto fail; > } > diff --git a/dump.c b/dump.c > index d9090a24cc..a79773d0f7 100644 > --- a/dump.c > +++ b/dump.c > @@ -1695,10 +1695,8 @@ static void dump_process(DumpState *s, Error **errp) > > static void *dump_thread(void *data) > { > - Error *err = NULL; > DumpState *s = (DumpState *)data; > - dump_process(s, &err); > - error_free(err); > + dump_process(s, NULL); > return NULL; > } > >
On Tue 29 Aug 2017 02:31:35 PM CEST, Philippe Mathieu-Daudé wrote: >> There's a few cases which we're passing an Error pointer to a function >> only to discard it immediately afterwards without checking it. In >> these cases we can simply remove the variable and pass NULL instead. > > How did you notice? I noticed it while I was working on something else. It's also not the first time I see something like this (see e.g. 026ac1586bdbd184e240). Berto
On 08/29/2017 07:08 AM, Alberto Garcia wrote: > There's a few cases which we're passing an Error pointer to a function > only to discard it immediately afterwards without checking it. In > these cases we can simply remove the variable and pass NULL instead. Could also go in via qemu-trivial or Markus' error tree (both added in cc) if the block maintainers don't take it quickly. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block/qcow.c | 12 +++--------- > block/qcow2.c | 8 ++------ > dump.c | 4 +--- > 3 files changed, 6 insertions(+), 18 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com>
On Tue 29 Aug 2017 04:40:35 PM CEST, Eric Blake wrote:
>> There's a few cases which we're passing an Error pointer to a
Hmmm... this was "few cases in which", but it seems that I accidentally
removed the "in". Not very important, but whoever commits this feel free
to fix the message.
Berto
On Tue, Aug 29, 2017 at 03:08:36PM +0300, Alberto Garcia wrote: > There's a few cases which we're passing an Error pointer to a function > only to discard it immediately afterwards without checking it. In > these cases we can simply remove the variable and pass NULL instead. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block/qcow.c | 12 +++--------- > block/qcow2.c | 8 ++------ > dump.c | 4 +--- > 3 files changed, 6 insertions(+), 18 deletions(-) Thanks, applied to my block-next tree: https://github.com/stefanha/qemu/commits/block-next Stefan
diff --git a/block/qcow.c b/block/qcow.c index c08cdc4a7b..63904a26ee 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -454,13 +454,11 @@ static uint64_t get_cluster_offset(BlockDriverState *bs, start_sect = (offset & ~(s->cluster_size - 1)) >> 9; for(i = 0; i < s->cluster_sectors; i++) { if (i < n_start || i >= n_end) { - Error *err = NULL; memset(s->cluster_data, 0x00, 512); if (qcrypto_block_encrypt(s->crypto, start_sect + i, s->cluster_data, BDRV_SECTOR_SIZE, - &err) < 0) { - error_free(err); + NULL) < 0) { errno = EIO; return -1; } @@ -572,7 +570,6 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num, QEMUIOVector hd_qiov; uint8_t *buf; void *orig_buf; - Error *err = NULL; if (qiov->niov > 1) { buf = orig_buf = qemu_try_blockalign(bs, qiov->size); @@ -637,7 +634,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num, if (bs->encrypted) { assert(s->crypto); if (qcrypto_block_decrypt(s->crypto, sector_num, buf, - n * BDRV_SECTOR_SIZE, &err) < 0) { + n * BDRV_SECTOR_SIZE, NULL) < 0) { goto fail; } } @@ -660,7 +657,6 @@ done: return ret; fail: - error_free(err); ret = -EIO; goto done; } @@ -709,11 +705,9 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num, break; } if (bs->encrypted) { - Error *err = NULL; assert(s->crypto); if (qcrypto_block_encrypt(s->crypto, sector_num, buf, - n * BDRV_SECTOR_SIZE, &err) < 0) { - error_free(err); + n * BDRV_SECTOR_SIZE, NULL) < 0) { ret = -EIO; break; } diff --git a/block/qcow2.c b/block/qcow2.c index 40ba26c111..fbfffadc76 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1820,15 +1820,13 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset, assert(s->crypto); assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0); assert((cur_bytes & (BDRV_SECTOR_SIZE - 1)) == 0); - Error *err = NULL; if (qcrypto_block_decrypt(s->crypto, (s->crypt_physical_offset ? cluster_offset + offset_in_cluster : offset) >> BDRV_SECTOR_BITS, cluster_data, cur_bytes, - &err) < 0) { - error_free(err); + NULL) < 0) { ret = -EIO; goto fail; } @@ -1942,7 +1940,6 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset, qemu_iovec_concat(&hd_qiov, qiov, bytes_done, cur_bytes); if (bs->encrypted) { - Error *err = NULL; assert(s->crypto); if (!cluster_data) { cluster_data = qemu_try_blockalign(bs->file->bs, @@ -1963,8 +1960,7 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset, cluster_offset + offset_in_cluster : offset) >> BDRV_SECTOR_BITS, cluster_data, - cur_bytes, &err) < 0) { - error_free(err); + cur_bytes, NULL) < 0) { ret = -EIO; goto fail; } diff --git a/dump.c b/dump.c index d9090a24cc..a79773d0f7 100644 --- a/dump.c +++ b/dump.c @@ -1695,10 +1695,8 @@ static void dump_process(DumpState *s, Error **errp) static void *dump_thread(void *data) { - Error *err = NULL; DumpState *s = (DumpState *)data; - dump_process(s, &err); - error_free(err); + dump_process(s, NULL); return NULL; }
There's a few cases which we're passing an Error pointer to a function only to discard it immediately afterwards without checking it. In these cases we can simply remove the variable and pass NULL instead. Signed-off-by: Alberto Garcia <berto@igalia.com> --- block/qcow.c | 12 +++--------- block/qcow2.c | 8 ++------ dump.c | 4 +--- 3 files changed, 6 insertions(+), 18 deletions(-)