* [PATCH v3 1/7] migration/rdma: Fix save_page method to fail on polling error
2023-09-21 12:13 [PATCH v3 0/7] Steps towards enabling -Wshadow=local Markus Armbruster
@ 2023-09-21 12:13 ` Markus Armbruster
2023-09-21 12:13 ` [PATCH v3 2/7] migration: Clean up local variable shadowing Markus Armbruster
` (6 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Markus Armbruster @ 2023-09-21 12:13 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, hreitz, eblake, vsementsov, jsnow, idryomov, pl, sw,
sstabellini, anthony.perard, paul, pbonzini, marcandre.lureau,
berrange, thuth, philmd, stefanha, fam, quintela, peterx, leobras,
kraxel, qemu-block, xen-devel, alex.bennee, peter.maydell,
Li Zhijian
qemu_rdma_save_page() reports polling error with error_report(), then
succeeds anyway. This is because the variable holding the polling
status *shadows* the variable the function returns. The latter
remains zero.
Broken since day one, and duplicated more recently.
Fixes: 2da776db4846 (rdma: core logic)
Fixes: b390afd8c50b (migration/rdma: Fix out of order wrid)
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Li Zhijian <lizhijian@fujitsu.com>
---
migration/rdma.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/migration/rdma.c b/migration/rdma.c
index a2a3db35b1..3915d1d7c9 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -3282,7 +3282,8 @@ static size_t qemu_rdma_save_page(QEMUFile *f,
*/
while (1) {
uint64_t wr_id, wr_id_in;
- int ret = qemu_rdma_poll(rdma, rdma->recv_cq, &wr_id_in, NULL);
+ ret = qemu_rdma_poll(rdma, rdma->recv_cq, &wr_id_in, NULL);
+
if (ret < 0) {
error_report("rdma migration: polling error! %d", ret);
goto err;
@@ -3297,7 +3298,8 @@ static size_t qemu_rdma_save_page(QEMUFile *f,
while (1) {
uint64_t wr_id, wr_id_in;
- int ret = qemu_rdma_poll(rdma, rdma->send_cq, &wr_id_in, NULL);
+ ret = qemu_rdma_poll(rdma, rdma->send_cq, &wr_id_in, NULL);
+
if (ret < 0) {
error_report("rdma migration: polling error! %d", ret);
goto err;
--
2.41.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v3 2/7] migration: Clean up local variable shadowing
2023-09-21 12:13 [PATCH v3 0/7] Steps towards enabling -Wshadow=local Markus Armbruster
2023-09-21 12:13 ` [PATCH v3 1/7] migration/rdma: Fix save_page method to fail on polling error Markus Armbruster
@ 2023-09-21 12:13 ` Markus Armbruster
2023-09-21 12:13 ` [PATCH v3 3/7] ui: " Markus Armbruster
` (5 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Markus Armbruster @ 2023-09-21 12:13 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, hreitz, eblake, vsementsov, jsnow, idryomov, pl, sw,
sstabellini, anthony.perard, paul, pbonzini, marcandre.lureau,
berrange, thuth, philmd, stefanha, fam, quintela, peterx, leobras,
kraxel, qemu-block, xen-devel, alex.bennee, peter.maydell,
Li Zhijian
Local variables shadowing other local variables or parameters make the
code needlessly hard to understand. Tracked down with -Wshadow=local.
Clean up: delete inner declarations when they are actually redundant,
else rename variables.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Li Zhijian <lizhijian@fujitsu.com>
---
migration/block.c | 4 ++--
migration/ram.c | 8 +++-----
migration/rdma.c | 8 +++++---
migration/vmstate.c | 2 +-
4 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/migration/block.c b/migration/block.c
index 86c2256a2b..eb6aafeb9e 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -440,8 +440,8 @@ static int init_blk_migration(QEMUFile *f)
/* Can only insert new BDSes now because doing so while iterating block
* devices may end up in a deadlock (iterating the new BDSes, too). */
for (i = 0; i < num_bs; i++) {
- BlkMigDevState *bmds = bmds_bs[i].bmds;
- BlockDriverState *bs = bmds_bs[i].bs;
+ bmds = bmds_bs[i].bmds;
+ bs = bmds_bs[i].bs;
if (bmds) {
ret = blk_insert_bs(bmds->blk, bs, &local_err);
diff --git a/migration/ram.c b/migration/ram.c
index 9040d66e61..0c202f8109 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3517,8 +3517,6 @@ int colo_init_ram_cache(void)
* we use the same name 'ram_bitmap' as for migration.
*/
if (ram_bytes_total()) {
- RAMBlock *block;
-
RAMBLOCK_FOREACH_NOT_IGNORED(block) {
unsigned long pages = block->max_length >> TARGET_PAGE_BITS;
block->bmap = bitmap_new(pages);
@@ -3998,12 +3996,12 @@ static int ram_load_precopy(QEMUFile *f)
}
}
if (migrate_ignore_shared()) {
- hwaddr addr = qemu_get_be64(f);
+ hwaddr addr2 = qemu_get_be64(f);
if (migrate_ram_is_ignored(block) &&
- block->mr->addr != addr) {
+ block->mr->addr != addr2) {
error_report("Mismatched GPAs for block %s "
"%" PRId64 "!= %" PRId64,
- id, (uint64_t)addr,
+ id, (uint64_t)addr2,
(uint64_t)block->mr->addr);
ret = -EINVAL;
}
diff --git a/migration/rdma.c b/migration/rdma.c
index 3915d1d7c9..c78ddfcb74 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -1902,9 +1902,11 @@ static int qemu_rdma_exchange_send(RDMAContext *rdma, RDMAControlHeader *head,
* by waiting for a READY message.
*/
if (rdma->control_ready_expected) {
- RDMAControlHeader resp;
- ret = qemu_rdma_exchange_get_response(rdma,
- &resp, RDMA_CONTROL_READY, RDMA_WRID_READY);
+ RDMAControlHeader resp_ignored;
+
+ ret = qemu_rdma_exchange_get_response(rdma, &resp_ignored,
+ RDMA_CONTROL_READY,
+ RDMA_WRID_READY);
if (ret < 0) {
return ret;
}
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 31842c3afb..438ea77cfa 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -97,7 +97,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
return -EINVAL;
}
if (vmsd->pre_load) {
- int ret = vmsd->pre_load(opaque);
+ ret = vmsd->pre_load(opaque);
if (ret) {
return ret;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v3 3/7] ui: Clean up local variable shadowing
2023-09-21 12:13 [PATCH v3 0/7] Steps towards enabling -Wshadow=local Markus Armbruster
2023-09-21 12:13 ` [PATCH v3 1/7] migration/rdma: Fix save_page method to fail on polling error Markus Armbruster
2023-09-21 12:13 ` [PATCH v3 2/7] migration: Clean up local variable shadowing Markus Armbruster
@ 2023-09-21 12:13 ` Markus Armbruster
2023-09-21 12:13 ` [PATCH v3 4/7] block/dirty-bitmap: " Markus Armbruster
` (4 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Markus Armbruster @ 2023-09-21 12:13 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, hreitz, eblake, vsementsov, jsnow, idryomov, pl, sw,
sstabellini, anthony.perard, paul, pbonzini, marcandre.lureau,
berrange, thuth, philmd, stefanha, fam, quintela, peterx, leobras,
kraxel, qemu-block, xen-devel, alex.bennee, peter.maydell
Local variables shadowing other local variables or parameters make the
code needlessly hard to understand. Tracked down with -Wshadow=local.
Clean up: delete inner declarations when they are actually redundant,
else rename variables.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
ui/gtk.c | 14 +++++++-------
ui/spice-display.c | 9 +++++----
ui/vnc-palette.c | 2 --
ui/vnc.c | 12 ++++++------
ui/vnc-enc-zrle.c.inc | 9 ++++-----
5 files changed, 22 insertions(+), 24 deletions(-)
diff --git a/ui/gtk.c b/ui/gtk.c
index e09f97a86b..3373427c9b 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -930,8 +930,8 @@ static gboolean gd_motion_event(GtkWidget *widget, GdkEventMotion *motion,
GdkMonitor *monitor = gdk_display_get_monitor_at_window(dpy, win);
GdkRectangle geometry;
- int x = (int)motion->x_root;
- int y = (int)motion->y_root;
+ int xr = (int)motion->x_root;
+ int yr = (int)motion->y_root;
gdk_monitor_get_geometry(monitor, &geometry);
@@ -942,13 +942,13 @@ static gboolean gd_motion_event(GtkWidget *widget, GdkEventMotion *motion,
* may still be only half way across the screen. Without
* this warp, the server pointer would thus appear to hit
* an invisible wall */
- if (x <= geometry.x || x - geometry.x >= geometry.width - 1 ||
- y <= geometry.y || y - geometry.y >= geometry.height - 1) {
+ if (xr <= geometry.x || xr - geometry.x >= geometry.width - 1 ||
+ yr <= geometry.y || yr - geometry.y >= geometry.height - 1) {
GdkDevice *dev = gdk_event_get_device((GdkEvent *)motion);
- x = geometry.x + geometry.width / 2;
- y = geometry.y + geometry.height / 2;
+ xr = geometry.x + geometry.width / 2;
+ yr = geometry.y + geometry.height / 2;
- gdk_device_warp(dev, screen, x, y);
+ gdk_device_warp(dev, screen, xr, yr);
s->last_set = FALSE;
return FALSE;
}
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 5cc47bd668..6eb98a5a5c 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -1081,15 +1081,16 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
}
if (render_cursor) {
- int x, y;
+ int ptr_x, ptr_y;
+
qemu_mutex_lock(&ssd->lock);
- x = ssd->ptr_x;
- y = ssd->ptr_y;
+ ptr_x = ssd->ptr_x;
+ ptr_y = ssd->ptr_y;
qemu_mutex_unlock(&ssd->lock);
egl_texture_blit(ssd->gls, &ssd->blit_fb, &ssd->guest_fb,
!y_0_top);
egl_texture_blend(ssd->gls, &ssd->blit_fb, &ssd->cursor_fb,
- !y_0_top, x, y, 1.0, 1.0);
+ !y_0_top, ptr_x, ptr_y, 1.0, 1.0);
glFlush();
}
diff --git a/ui/vnc-palette.c b/ui/vnc-palette.c
index dc7c0ba997..4e88c412f0 100644
--- a/ui/vnc-palette.c
+++ b/ui/vnc-palette.c
@@ -86,8 +86,6 @@ int palette_put(VncPalette *palette, uint32_t color)
return 0;
}
if (!entry) {
- VncPaletteEntry *entry;
-
entry = &palette->pool[palette->size];
entry->color = color;
entry->idx = idx;
diff --git a/ui/vnc.c b/ui/vnc.c
index 6fd86996a5..ecb75ff8c8 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1584,15 +1584,15 @@ static void vnc_jobs_bh(void *opaque)
*/
static int vnc_client_read(VncState *vs)
{
- size_t ret;
+ size_t sz;
#ifdef CONFIG_VNC_SASL
if (vs->sasl.conn && vs->sasl.runSSF)
- ret = vnc_client_read_sasl(vs);
+ sz = vnc_client_read_sasl(vs);
else
#endif /* CONFIG_VNC_SASL */
- ret = vnc_client_read_plain(vs);
- if (!ret) {
+ sz = vnc_client_read_plain(vs);
+ if (!sz) {
if (vs->disconnecting) {
vnc_disconnect_finish(vs);
return -1;
@@ -3118,8 +3118,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
cmp_bytes = MIN(VNC_DIRTY_PIXELS_PER_BIT * VNC_SERVER_FB_BYTES,
server_stride);
if (vd->guest.format != VNC_SERVER_FB_FORMAT) {
- int width = pixman_image_get_width(vd->server);
- tmpbuf = qemu_pixman_linebuf_create(VNC_SERVER_FB_FORMAT, width);
+ int w = pixman_image_get_width(vd->server);
+ tmpbuf = qemu_pixman_linebuf_create(VNC_SERVER_FB_FORMAT, w);
} else {
int guest_bpp =
PIXMAN_FORMAT_BPP(pixman_image_get_format(vd->guest.fb));
diff --git a/ui/vnc-enc-zrle.c.inc b/ui/vnc-enc-zrle.c.inc
index a8ca37d05e..2ef7501d52 100644
--- a/ui/vnc-enc-zrle.c.inc
+++ b/ui/vnc-enc-zrle.c.inc
@@ -153,11 +153,12 @@ static void ZRLE_ENCODE_TILE(VncState *vs, ZRLE_PIXEL *data, int w, int h,
}
if (use_rle) {
- ZRLE_PIXEL *ptr = data;
- ZRLE_PIXEL *end = ptr + w * h;
ZRLE_PIXEL *run_start;
ZRLE_PIXEL pix;
+ ptr = data;
+ end = ptr + w * h;
+
while (ptr < end) {
int len;
int index = 0;
@@ -198,7 +199,7 @@ static void ZRLE_ENCODE_TILE(VncState *vs, ZRLE_PIXEL *data, int w, int h,
}
} else if (use_palette) { /* no RLE */
int bppp;
- ZRLE_PIXEL *ptr = data;
+ ptr = data;
/* packed pixels */
@@ -241,8 +242,6 @@ static void ZRLE_ENCODE_TILE(VncState *vs, ZRLE_PIXEL *data, int w, int h,
#endif
{
#ifdef ZRLE_COMPACT_PIXEL
- ZRLE_PIXEL *ptr;
-
for (ptr = data; ptr < data + w * h; ptr++) {
ZRLE_WRITE_PIXEL(vs, *ptr);
}
--
2.41.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v3 4/7] block/dirty-bitmap: Clean up local variable shadowing
2023-09-21 12:13 [PATCH v3 0/7] Steps towards enabling -Wshadow=local Markus Armbruster
` (2 preceding siblings ...)
2023-09-21 12:13 ` [PATCH v3 3/7] ui: " Markus Armbruster
@ 2023-09-21 12:13 ` Markus Armbruster
2023-09-21 12:13 ` [PATCH v3 5/7] block/vdi: " Markus Armbruster
` (3 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Markus Armbruster @ 2023-09-21 12:13 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, hreitz, eblake, vsementsov, jsnow, idryomov, pl, sw,
sstabellini, anthony.perard, paul, pbonzini, marcandre.lureau,
berrange, thuth, philmd, stefanha, fam, quintela, peterx, leobras,
kraxel, qemu-block, xen-devel, alex.bennee, peter.maydell
Local variables shadowing other local variables or parameters make the
code needlessly hard to understand. Tracked down with -Wshadow=local.
Clean up: rename both the pair of parameters and the pair of local
variables. While there, move the local variables to function scope.
Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block/monitor/bitmap-qmp-cmds.c | 19 ++++++++++---------
block/qcow2-bitmap.c | 3 +--
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
index 55f778f5af..70d01a3776 100644
--- a/block/monitor/bitmap-qmp-cmds.c
+++ b/block/monitor/bitmap-qmp-cmds.c
@@ -258,37 +258,38 @@ void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
bdrv_disable_dirty_bitmap(bitmap);
}
-BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
+BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *dst_node,
+ const char *dst_bitmap,
BlockDirtyBitmapOrStrList *bms,
HBitmap **backup, Error **errp)
{
BlockDriverState *bs;
BdrvDirtyBitmap *dst, *src;
BlockDirtyBitmapOrStrList *lst;
+ const char *src_node, *src_bitmap;
HBitmap *local_backup = NULL;
GLOBAL_STATE_CODE();
- dst = block_dirty_bitmap_lookup(node, target, &bs, errp);
+ dst = block_dirty_bitmap_lookup(dst_node, dst_bitmap, &bs, errp);
if (!dst) {
return NULL;
}
for (lst = bms; lst; lst = lst->next) {
switch (lst->value->type) {
- const char *name, *node;
case QTYPE_QSTRING:
- name = lst->value->u.local;
- src = bdrv_find_dirty_bitmap(bs, name);
+ src_bitmap = lst->value->u.local;
+ src = bdrv_find_dirty_bitmap(bs, src_bitmap);
if (!src) {
- error_setg(errp, "Dirty bitmap '%s' not found", name);
+ error_setg(errp, "Dirty bitmap '%s' not found", src_bitmap);
goto fail;
}
break;
case QTYPE_QDICT:
- node = lst->value->u.external.node;
- name = lst->value->u.external.name;
- src = block_dirty_bitmap_lookup(node, name, NULL, errp);
+ src_node = lst->value->u.external.node;
+ src_bitmap = lst->value->u.external.name;
+ src = block_dirty_bitmap_lookup(src_node, src_bitmap, NULL, errp);
if (!src) {
goto fail;
}
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 037fa2d435..ffd5cd3b23 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1555,7 +1555,6 @@ bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
const char *name = bdrv_dirty_bitmap_name(bitmap);
uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
- Qcow2Bitmap *bm;
if (!bdrv_dirty_bitmap_get_persistence(bitmap) ||
bdrv_dirty_bitmap_inconsistent(bitmap)) {
@@ -1625,7 +1624,7 @@ bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
/* allocate clusters and store bitmaps */
QSIMPLEQ_FOREACH(bm, bm_list, entry) {
- BdrvDirtyBitmap *bitmap = bm->dirty_bitmap;
+ bitmap = bm->dirty_bitmap;
if (bitmap == NULL || bdrv_dirty_bitmap_readonly(bitmap)) {
continue;
--
2.41.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v3 5/7] block/vdi: Clean up local variable shadowing
2023-09-21 12:13 [PATCH v3 0/7] Steps towards enabling -Wshadow=local Markus Armbruster
` (3 preceding siblings ...)
2023-09-21 12:13 ` [PATCH v3 4/7] block/dirty-bitmap: " Markus Armbruster
@ 2023-09-21 12:13 ` Markus Armbruster
2023-09-21 12:13 ` [PATCH v3 6/7] block: " Markus Armbruster
` (2 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Markus Armbruster @ 2023-09-21 12:13 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, hreitz, eblake, vsementsov, jsnow, idryomov, pl, sw,
sstabellini, anthony.perard, paul, pbonzini, marcandre.lureau,
berrange, thuth, philmd, stefanha, fam, quintela, peterx, leobras,
kraxel, qemu-block, xen-devel, alex.bennee, peter.maydell
Local variables shadowing other local variables or parameters make the
code needlessly hard to understand. Tracked down with -Wshadow=local.
Clean up: delete inner declarations when they are actually redundant,
else rename variables.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block/vdi.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/block/vdi.c b/block/vdi.c
index 6c35309e04..934e1b849b 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -634,7 +634,6 @@ vdi_co_pwritev(BlockDriverState *bs, int64_t offset, int64_t bytes,
bmap_entry = le32_to_cpu(s->bmap[block_index]);
if (!VDI_IS_ALLOCATED(bmap_entry)) {
/* Allocate new block and write to it. */
- uint64_t data_offset;
qemu_co_rwlock_upgrade(&s->bmap_lock);
bmap_entry = le32_to_cpu(s->bmap[block_index]);
if (VDI_IS_ALLOCATED(bmap_entry)) {
@@ -700,7 +699,7 @@ nonallocating_write:
/* One or more new blocks were allocated. */
VdiHeader *header;
uint8_t *base;
- uint64_t offset;
+ uint64_t bmap_offset;
uint32_t n_sectors;
g_free(block);
@@ -723,11 +722,11 @@ nonallocating_write:
bmap_first /= (SECTOR_SIZE / sizeof(uint32_t));
bmap_last /= (SECTOR_SIZE / sizeof(uint32_t));
n_sectors = bmap_last - bmap_first + 1;
- offset = s->bmap_sector + bmap_first;
+ bmap_offset = s->bmap_sector + bmap_first;
base = ((uint8_t *)&s->bmap[0]) + bmap_first * SECTOR_SIZE;
logout("will write %u block map sectors starting from entry %u\n",
n_sectors, bmap_first);
- ret = bdrv_co_pwrite(bs->file, offset * SECTOR_SIZE,
+ ret = bdrv_co_pwrite(bs->file, bmap_offset * SECTOR_SIZE,
n_sectors * SECTOR_SIZE, base, 0);
}
--
2.41.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v3 6/7] block: Clean up local variable shadowing
2023-09-21 12:13 [PATCH v3 0/7] Steps towards enabling -Wshadow=local Markus Armbruster
` (4 preceding siblings ...)
2023-09-21 12:13 ` [PATCH v3 5/7] block/vdi: " Markus Armbruster
@ 2023-09-21 12:13 ` Markus Armbruster
2023-09-21 12:13 ` [PATCH v3 7/7] qobject atomics osdep: Make a few macros more hygienic Markus Armbruster
2023-09-29 5:56 ` [PATCH v3 0/7] Steps towards enabling -Wshadow=local Markus Armbruster
7 siblings, 0 replies; 10+ messages in thread
From: Markus Armbruster @ 2023-09-21 12:13 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, hreitz, eblake, vsementsov, jsnow, idryomov, pl, sw,
sstabellini, anthony.perard, paul, pbonzini, marcandre.lureau,
berrange, thuth, philmd, stefanha, fam, quintela, peterx, leobras,
kraxel, qemu-block, xen-devel, alex.bennee, peter.maydell
Local variables shadowing other local variables or parameters make the
code needlessly hard to understand. Tracked down with -Wshadow=local.
Clean up: delete inner declarations when they are actually redundant,
else rename variables.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Ilya Dryomov <idryomov@gmail.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 9 +++++----
block/rbd.c | 2 +-
block/stream.c | 1 -
block/vvfat.c | 35 ++++++++++++++++++-----------------
hw/block/xen-block.c | 6 +++---
5 files changed, 27 insertions(+), 26 deletions(-)
diff --git a/block.c b/block.c
index 8da89aaa62..bb5dd17e9c 100644
--- a/block.c
+++ b/block.c
@@ -3035,18 +3035,19 @@ static BdrvChild *bdrv_attach_child_common(BlockDriverState *child_bs,
&local_err);
if (ret < 0 && child_class->change_aio_ctx) {
- Transaction *tran = tran_new();
+ Transaction *aio_ctx_tran = tran_new();
GHashTable *visited = g_hash_table_new(NULL, NULL);
bool ret_child;
g_hash_table_add(visited, new_child);
ret_child = child_class->change_aio_ctx(new_child, child_ctx,
- visited, tran, NULL);
+ visited, aio_ctx_tran,
+ NULL);
if (ret_child == true) {
error_free(local_err);
ret = 0;
}
- tran_finalize(tran, ret_child == true ? 0 : -1);
+ tran_finalize(aio_ctx_tran, ret_child == true ? 0 : -1);
g_hash_table_destroy(visited);
}
@@ -6077,12 +6078,12 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
QLIST_FOREACH(drv, &bdrv_drivers, list) {
if (drv->format_name) {
bool found = false;
- int i = count;
if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, read_only)) {
continue;
}
+ i = count;
while (formats && i && !found) {
found = !strcmp(formats[--i], drv->format_name);
}
diff --git a/block/rbd.c b/block/rbd.c
index 978671411e..472ca05cba 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -1290,7 +1290,7 @@ static int coroutine_fn qemu_rbd_start_co(BlockDriverState *bs,
* operations that exceed the current size.
*/
if (offset + bytes > s->image_size) {
- int r = qemu_rbd_resize(bs, offset + bytes);
+ r = qemu_rbd_resize(bs, offset + bytes);
if (r < 0) {
return r;
}
diff --git a/block/stream.c b/block/stream.c
index e522bbdec5..007253880b 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -282,7 +282,6 @@ void stream_start(const char *job_id, BlockDriverState *bs,
/* Make sure that the image is opened in read-write mode */
bs_read_only = bdrv_is_read_only(bs);
if (bs_read_only) {
- int ret;
/* Hold the chain during reopen */
if (bdrv_freeze_backing_chain(bs, above_base, errp) < 0) {
return;
diff --git a/block/vvfat.c b/block/vvfat.c
index 0ddc91fc09..856b479c91 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -777,7 +777,6 @@ static int read_directory(BDRVVVFATState* s, int mapping_index)
while((entry=readdir(dir))) {
unsigned int length=strlen(dirname)+2+strlen(entry->d_name);
char* buffer;
- direntry_t* direntry;
struct stat st;
int is_dot=!strcmp(entry->d_name,".");
int is_dotdot=!strcmp(entry->d_name,"..");
@@ -857,7 +856,7 @@ static int read_directory(BDRVVVFATState* s, int mapping_index)
/* fill with zeroes up to the end of the cluster */
while(s->directory.next%(0x10*s->sectors_per_cluster)) {
- direntry_t* direntry=array_get_next(&(s->directory));
+ direntry = array_get_next(&(s->directory));
memset(direntry,0,sizeof(direntry_t));
}
@@ -1962,24 +1961,24 @@ get_cluster_count_for_direntry(BDRVVVFATState* s, direntry_t* direntry, const ch
* This is horribly inefficient, but that is okay, since
* it is rarely executed, if at all.
*/
- int64_t offset = cluster2sector(s, cluster_num);
+ int64_t offs = cluster2sector(s, cluster_num);
vvfat_close_current_file(s);
for (i = 0; i < s->sectors_per_cluster; i++) {
int res;
res = bdrv_is_allocated(s->qcow->bs,
- (offset + i) * BDRV_SECTOR_SIZE,
+ (offs + i) * BDRV_SECTOR_SIZE,
BDRV_SECTOR_SIZE, NULL);
if (res < 0) {
return -1;
}
if (!res) {
- res = vvfat_read(s->bs, offset, s->cluster_buffer, 1);
+ res = vvfat_read(s->bs, offs, s->cluster_buffer, 1);
if (res) {
return -1;
}
- res = bdrv_co_pwrite(s->qcow, offset * BDRV_SECTOR_SIZE,
+ res = bdrv_co_pwrite(s->qcow, offs * BDRV_SECTOR_SIZE,
BDRV_SECTOR_SIZE, s->cluster_buffer,
0);
if (res < 0) {
@@ -2467,8 +2466,9 @@ commit_direntries(BDRVVVFATState* s, int dir_index, int parent_mapping_index)
for (c = first_cluster; !fat_eof(s, c); c = modified_fat_get(s, c)) {
direntry_t *first_direntry;
- void* direntry = array_get(&(s->directory), current_dir_index);
- int ret = vvfat_read(s->bs, cluster2sector(s, c), direntry,
+
+ direntry = array_get(&(s->directory), current_dir_index);
+ ret = vvfat_read(s->bs, cluster2sector(s, c), (uint8_t *)direntry,
s->sectors_per_cluster);
if (ret)
return ret;
@@ -2690,12 +2690,12 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
direntry_t* direntry = array_get(&(s->directory),
mapping->info.dir.first_dir_index);
uint32_t c = mapping->begin;
- int i = 0;
+ int j = 0;
/* recurse */
while (!fat_eof(s, c)) {
do {
- direntry_t* d = direntry + i;
+ direntry_t *d = direntry + j;
if (is_file(d) || (is_directory(d) && !is_dot(d))) {
int l;
@@ -2716,8 +2716,8 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
schedule_rename(s, m->begin, new_path);
}
- i++;
- } while((i % (0x10 * s->sectors_per_cluster)) != 0);
+ j++;
+ } while (j % (0x10 * s->sectors_per_cluster) != 0);
c = fat_get(s, c);
}
}
@@ -2804,16 +2804,16 @@ static int coroutine_fn GRAPH_RDLOCK handle_commits(BDRVVVFATState* s)
int begin = commit->param.new_file.first_cluster;
mapping_t* mapping = find_mapping_for_cluster(s, begin);
direntry_t* entry;
- int i;
+ int j;
/* find direntry */
- for (i = 0; i < s->directory.next; i++) {
- entry = array_get(&(s->directory), i);
+ for (j = 0; j < s->directory.next; j++) {
+ entry = array_get(&(s->directory), j);
if (is_file(entry) && begin_of_direntry(entry) == begin)
break;
}
- if (i >= s->directory.next) {
+ if (j >= s->directory.next) {
fail = -6;
continue;
}
@@ -2833,8 +2833,9 @@ static int coroutine_fn GRAPH_RDLOCK handle_commits(BDRVVVFATState* s)
mapping->mode = MODE_NORMAL;
mapping->info.file.offset = 0;
- if (commit_one_file(s, i, 0))
+ if (commit_one_file(s, j, 0)) {
fail = -7;
+ }
break;
}
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 3906b9058b..a07cd7eb5d 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -369,7 +369,7 @@ static void xen_block_get_vdev(Object *obj, Visitor *v, const char *name,
case XEN_BLOCK_VDEV_TYPE_XVD:
case XEN_BLOCK_VDEV_TYPE_HD:
case XEN_BLOCK_VDEV_TYPE_SD: {
- char *name = disk_to_vbd_name(vdev->disk);
+ char *vbd_name = disk_to_vbd_name(vdev->disk);
str = g_strdup_printf("%s%s%lu",
(vdev->type == XEN_BLOCK_VDEV_TYPE_XVD) ?
@@ -377,8 +377,8 @@ static void xen_block_get_vdev(Object *obj, Visitor *v, const char *name,
(vdev->type == XEN_BLOCK_VDEV_TYPE_HD) ?
"hd" :
"sd",
- name, vdev->partition);
- g_free(name);
+ vbd_name, vdev->partition);
+ g_free(vbd_name);
break;
}
default:
--
2.41.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v3 7/7] qobject atomics osdep: Make a few macros more hygienic
2023-09-21 12:13 [PATCH v3 0/7] Steps towards enabling -Wshadow=local Markus Armbruster
` (5 preceding siblings ...)
2023-09-21 12:13 ` [PATCH v3 6/7] block: " Markus Armbruster
@ 2023-09-21 12:13 ` Markus Armbruster
2023-09-21 12:41 ` Philippe Mathieu-Daudé
2023-09-29 5:56 ` [PATCH v3 0/7] Steps towards enabling -Wshadow=local Markus Armbruster
7 siblings, 1 reply; 10+ messages in thread
From: Markus Armbruster @ 2023-09-21 12:13 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, hreitz, eblake, vsementsov, jsnow, idryomov, pl, sw,
sstabellini, anthony.perard, paul, pbonzini, marcandre.lureau,
berrange, thuth, philmd, stefanha, fam, quintela, peterx, leobras,
kraxel, qemu-block, xen-devel, alex.bennee, peter.maydell
Variables declared in macros can shadow other variables. Much of the
time, this is harmless, e.g.:
#define _FDT(exp) \
do { \
int ret = (exp); \
if (ret < 0) { \
error_report("error creating device tree: %s: %s", \
#exp, fdt_strerror(ret)); \
exit(1); \
} \
} while (0)
Harmless shadowing in h_client_architecture_support():
target_ulong ret;
[...]
ret = do_client_architecture_support(cpu, spapr, vec, fdt_bufsize);
if (ret == H_SUCCESS) {
_FDT((fdt_pack(spapr->fdt_blob)));
[...]
}
return ret;
However, we can get in trouble when the shadowed variable is used in a
macro argument:
#define QOBJECT(obj) ({ \
typeof(obj) o = (obj); \
o ? container_of(&(o)->base, QObject, base) : NULL; \
})
QOBJECT(o) expands into
({
---> typeof(o) o = (o);
o ? container_of(&(o)->base, QObject, base) : NULL;
})
Unintended variable name capture at --->. We'd be saved by
-Winit-self. But I could certainly construct more elaborate death
traps that don't trigger it.
To reduce the risk of trapping ourselves, we use variable names in
macros that no sane person would use elsewhere. Here's our actual
definition of QOBJECT():
#define QOBJECT(obj) ({ \
typeof(obj) _obj = (obj); \
_obj ? container_of(&(_obj)->base, QObject, base) : NULL; \
})
Works well enough until we nest macro calls. For instance, with
#define qobject_ref(obj) ({ \
typeof(obj) _obj = (obj); \
qobject_ref_impl(QOBJECT(_obj)); \
_obj; \
})
the expression qobject_ref(obj) expands into
({
typeof(obj) _obj = (obj);
qobject_ref_impl(
({
---> typeof(_obj) _obj = (_obj);
_obj ? container_of(&(_obj)->base, QObject, base) : NULL;
}));
_obj;
})
Unintended variable name capture at --->.
The only reliable way to prevent unintended variable name capture is
-Wshadow.
One blocker for enabling it is shadowing hiding in function-like
macros like
qdict_put(dict, "name", qobject_ref(...))
qdict_put() wraps its last argument in QOBJECT(), and the last
argument here contains another QOBJECT().
Use dark preprocessor sorcery to make the macros that give us this
problem use different variable names on every call.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
include/qapi/qmp/qobject.h | 10 ++++++++--
include/qemu/atomic.h | 17 ++++++++++++-----
include/qemu/compiler.h | 3 +++
include/qemu/osdep.h | 27 ++++++++++++++++++++-------
4 files changed, 43 insertions(+), 14 deletions(-)
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index 9003b71fd3..89b97d88bc 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -45,10 +45,16 @@ struct QObject {
struct QObjectBase_ base;
};
-#define QOBJECT(obj) ({ \
+/*
+ * Preprocessor sorcery ahead: use a different identifier for the
+ * local variable in each expansion, so we can nest macro calls
+ * without shadowing variables.
+ */
+#define QOBJECT_INTERNAL(obj, _obj) ({ \
typeof(obj) _obj = (obj); \
- _obj ? container_of(&(_obj)->base, QObject, base) : NULL; \
+ _obj ? container_of(&_obj->base, QObject, base) : NULL; \
})
+#define QOBJECT(obj) QOBJECT_INTERNAL((obj), MAKE_IDENTFIER(_obj))
/* Required for qobject_to() */
#define QTYPE_CAST_TO_QNull QTYPE_QNULL
diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index d95612f7a0..f1d3d1702a 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -157,13 +157,20 @@
smp_read_barrier_depends();
#endif
-#define qatomic_rcu_read(ptr) \
- ({ \
+/*
+ * Preprocessor sorcery ahead: use a different identifier for the
+ * local variable in each expansion, so we can nest macro calls
+ * without shadowing variables.
+ */
+#define qatomic_rcu_read_internal(ptr, _val) \
+ ({ \
qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \
- typeof_strip_qual(*ptr) _val; \
- qatomic_rcu_read__nocheck(ptr, &_val); \
- _val; \
+ typeof_strip_qual(*ptr) _val; \
+ qatomic_rcu_read__nocheck(ptr, &_val); \
+ _val; \
})
+#define qatomic_rcu_read(ptr) \
+ qatomic_rcu_read_internal((ptr), MAKE_IDENTFIER(_val))
#define qatomic_rcu_set(ptr, i) do { \
qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \
diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index a309f90c76..03236d830c 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -37,6 +37,9 @@
#define tostring(s) #s
#endif
+/* Expands into an identifier stemN, where N is another number each time */
+#define MAKE_IDENTFIER(stem) glue(stem, __COUNTER__)
+
#ifndef likely
#define likely(x) __builtin_expect(!!(x), 1)
#define unlikely(x) __builtin_expect(!!(x), 0)
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 2897720fac..a80f49d1ab 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -383,19 +383,28 @@ void QEMU_ERROR("code path is reachable")
* determined by the pre-processor instead of the compiler, you'll
* have to open-code it. Sadly, Coverity is severely confused by the
* constant variants, so we have to dumb things down there.
+ *
+ * Preprocessor sorcery ahead: use different identifiers for the local
+ * variables in each expansion, so we can nest macro calls without
+ * shadowing variables.
*/
-#undef MIN
-#define MIN(a, b) \
+#define MIN_INTERNAL(a, b, _a, _b) \
({ \
typeof(1 ? (a) : (b)) _a = (a), _b = (b); \
_a < _b ? _a : _b; \
})
-#undef MAX
-#define MAX(a, b) \
+#undef MIN
+#define MIN(a, b) \
+ MIN_INTERNAL((a), (b), MAKE_IDENTFIER(_a), MAKE_IDENTFIER(_b))
+
+#define MAX_INTERNAL(a, b, _a, _b) \
({ \
typeof(1 ? (a) : (b)) _a = (a), _b = (b); \
_a > _b ? _a : _b; \
})
+#undef MAX
+#define MAX(a, b) \
+ MAX_INTERNAL((a), (b), MAKE_IDENTFIER(_a), MAKE_IDENTFIER(_b))
#ifdef __COVERITY__
# define MIN_CONST(a, b) ((a) < (b) ? (a) : (b))
@@ -416,14 +425,18 @@ void QEMU_ERROR("code path is reachable")
/*
* Minimum function that returns zero only if both values are zero.
* Intended for use with unsigned values only.
+ *
+ * Preprocessor sorcery ahead: use different identifiers for the local
+ * variables in each expansion, so we can nest macro calls without
+ * shadowing variables.
*/
-#ifndef MIN_NON_ZERO
-#define MIN_NON_ZERO(a, b) \
+#define MIN_NON_ZERO_INTERNAL(a, b, _a, _b) \
({ \
typeof(1 ? (a) : (b)) _a = (a), _b = (b); \
_a == 0 ? _b : (_b == 0 || _b > _a) ? _a : _b; \
})
-#endif
+#define MIN_NON_ZERO(a, b) \
+ MIN_NON_ZERO_INTERNAL((a), (b), MAKE_IDENTFIER(_a), MAKE_IDENTFIER(_b))
/*
* Round number down to multiple. Safe when m is not a power of 2 (see
--
2.41.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v3 7/7] qobject atomics osdep: Make a few macros more hygienic
2023-09-21 12:13 ` [PATCH v3 7/7] qobject atomics osdep: Make a few macros more hygienic Markus Armbruster
@ 2023-09-21 12:41 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-09-21 12:41 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: kwolf, hreitz, eblake, vsementsov, jsnow, idryomov, pl, sw,
sstabellini, anthony.perard, paul, pbonzini, marcandre.lureau,
berrange, thuth, stefanha, fam, quintela, peterx, leobras, kraxel,
qemu-block, xen-devel, alex.bennee, peter.maydell
On 21/9/23 14:13, Markus Armbruster wrote:
> Variables declared in macros can shadow other variables. Much of the
> time, this is harmless, e.g.:
>
> #define _FDT(exp) \
> do { \
> int ret = (exp); \
> if (ret < 0) { \
> error_report("error creating device tree: %s: %s", \
> #exp, fdt_strerror(ret)); \
> exit(1); \
> } \
> } while (0)
>
> Harmless shadowing in h_client_architecture_support():
>
> target_ulong ret;
>
> [...]
>
> ret = do_client_architecture_support(cpu, spapr, vec, fdt_bufsize);
> if (ret == H_SUCCESS) {
> _FDT((fdt_pack(spapr->fdt_blob)));
> [...]
> }
>
> return ret;
>
> However, we can get in trouble when the shadowed variable is used in a
> macro argument:
>
> #define QOBJECT(obj) ({ \
> typeof(obj) o = (obj); \
> o ? container_of(&(o)->base, QObject, base) : NULL; \
> })
>
> QOBJECT(o) expands into
>
> ({
> ---> typeof(o) o = (o);
> o ? container_of(&(o)->base, QObject, base) : NULL;
> })
>
> Unintended variable name capture at --->. We'd be saved by
> -Winit-self. But I could certainly construct more elaborate death
> traps that don't trigger it.
>
> To reduce the risk of trapping ourselves, we use variable names in
> macros that no sane person would use elsewhere. Here's our actual
> definition of QOBJECT():
>
> #define QOBJECT(obj) ({ \
> typeof(obj) _obj = (obj); \
> _obj ? container_of(&(_obj)->base, QObject, base) : NULL; \
> })
>
> Works well enough until we nest macro calls. For instance, with
>
> #define qobject_ref(obj) ({ \
> typeof(obj) _obj = (obj); \
> qobject_ref_impl(QOBJECT(_obj)); \
> _obj; \
> })
>
> the expression qobject_ref(obj) expands into
>
> ({
> typeof(obj) _obj = (obj);
> qobject_ref_impl(
> ({
> ---> typeof(_obj) _obj = (_obj);
> _obj ? container_of(&(_obj)->base, QObject, base) : NULL;
> }));
> _obj;
> })
>
> Unintended variable name capture at --->.
>
> The only reliable way to prevent unintended variable name capture is
> -Wshadow.
>
> One blocker for enabling it is shadowing hiding in function-like
> macros like
>
> qdict_put(dict, "name", qobject_ref(...))
>
> qdict_put() wraps its last argument in QOBJECT(), and the last
> argument here contains another QOBJECT().
>
> Use dark preprocessor sorcery to make the macros that give us this
> problem use different variable names on every call.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> include/qapi/qmp/qobject.h | 10 ++++++++--
> include/qemu/atomic.h | 17 ++++++++++++-----
> include/qemu/compiler.h | 3 +++
> include/qemu/osdep.h | 27 ++++++++++++++++++++-------
> 4 files changed, 43 insertions(+), 14 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 0/7] Steps towards enabling -Wshadow=local
2023-09-21 12:13 [PATCH v3 0/7] Steps towards enabling -Wshadow=local Markus Armbruster
` (6 preceding siblings ...)
2023-09-21 12:13 ` [PATCH v3 7/7] qobject atomics osdep: Make a few macros more hygienic Markus Armbruster
@ 2023-09-29 5:56 ` Markus Armbruster
7 siblings, 0 replies; 10+ messages in thread
From: Markus Armbruster @ 2023-09-29 5:56 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, hreitz, eblake, vsementsov, jsnow, idryomov, sw,
sstabellini, anthony.perard, paul, pbonzini, marcandre.lureau,
berrange, thuth, philmd, stefanha, fam, quintela, peterx, leobras,
kraxel, qemu-block, xen-devel, alex.bennee, peter.maydell
Markus Armbruster <armbru@redhat.com> writes:
> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand. Bugs love to hide in such code.
> Evidence: PATCH 1.
>
> Enabling -Wshadow would prevent bugs like this one. But we'd have to
> clean up all the offenders first. We got a lot of them.
>
> Enabling -Wshadow=local should be less work for almost as much gain.
> I took a stab at it. There's a small, exciting part, and a large,
> boring part.
>
> The exciting part is dark preprocessor sorcery to let us nest macro
> calls without shadowing: PATCH 7.
[...]
Queued.
^ permalink raw reply [flat|nested] 10+ messages in thread