[PATCH v3 00/22] -Werror=maybe-uninitialized fixes

[PATCH v3 00/22] -Werror=maybe-uninitialized fixes

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

Depending on -Doptimization=<value>, GCC (14.2.1 here) produces different
maybe-uninitialized warnings:
- g: produces -Werror=maybe-uninitialized errors
- 0: clean build
- 1: produces -Werror=maybe-uninitialized errors
- 2: clean build
- 3: produces few -Werror=maybe-uninitialized errors
- s: produces -Werror=maybe-uninitialized errors

Most are false-positive, because prior LOCK_GUARD should guarantee an
initialization path. Few of them are a bit trickier. Finally, I found
a potential related memory leak.

Patches missing r-b: 6, 10-11, 15-21

thanks

v3:
 - some changes suggested by Manos
 - added s-o-b/a-b

v2:
 - rebased, dropped some patches
 - added some new patches with updated code-base and newer GCC
 - added s-o-b/a-b

Marc-André Lureau (22):
  util/coroutine: fix -Werror=maybe-uninitialized false-positive
  util/timer: fix -Werror=maybe-uninitialized false-positive
  hw/qxl: fix -Werror=maybe-uninitialized false-positives
  nbd: fix -Werror=maybe-uninitialized false-positive
  block/mirror: fix -Werror=maybe-uninitialized false-positive
  block/mirror: fix -Werror=maybe-uninitialized false-positive
  block/stream: fix -Werror=maybe-uninitialized false-positives
  hw/ahci: fix -Werror=maybe-uninitialized false-positive
  hw/vhost-scsi: fix -Werror=maybe-uninitialized
  hw/sdhci: fix -Werror=maybe-uninitialized false-positive
  block/block-copy: fix -Werror=maybe-uninitialized false-positive
  migration: fix -Werror=maybe-uninitialized false-positives
  hw/virtio-blk: fix -Werror=maybe-uninitialized false-positive
  migration: fix -Werror=maybe-uninitialized false-positive
  linux-user/hppa: fix -Werror=maybe-uninitialized false-positive
  target/loongarch: fix -Werror=maybe-uninitialized false-positive
  tests: fix -Werror=maybe-uninitialized false-positive
  hw/virtio: fix -Werror=maybe-uninitialized
  RFC: hw/virtio: a potential leak fix
  block: fix -Werror=maybe-uninitialized false-positive
  qom/object: fix -Werror=maybe-uninitialized
  fsdep/9p: fix -Werror=maybe-uninitialized false-positive

 block/block-copy.c                 |  2 +-
 block/file-posix.c                 |  2 +-
 block/mirror.c                     |  8 ++++----
 block/stream.c                     |  6 +++---
 fsdev/9p-iov-marshal.c             |  6 +++---
 hw/block/virtio-blk.c              |  2 +-
 hw/display/qxl.c                   |  4 ++--
 hw/ide/ahci.c                      |  3 ++-
 hw/scsi/vhost-scsi.c               |  2 +-
 hw/sd/sdhci.c                      |  2 +-
 hw/virtio/vhost-shadow-virtqueue.c |  6 ++++--
 linux-user/hppa/cpu_loop.c         | 10 +++++-----
 migration/dirtyrate.c              |  4 ++--
 migration/migration.c              |  2 +-
 migration/ram.c                    |  2 +-
 nbd/client-connection.c            |  2 +-
 qom/object.c                       |  5 ++++-
 target/loongarch/gdbstub.c         | 26 ++++++++++++++------------
 tests/unit/test-bdrv-drain.c       |  2 +-
 tests/unit/test-block-iothread.c   |  2 +-
 util/qemu-coroutine.c              |  2 +-
 util/qemu-timer.c                  |  6 +++---
 roms/openbios                      |  2 +-
 23 files changed, 58 insertions(+), 50 deletions(-)

-- 
2.45.2.827.g557ae147e6

[PATCH v3 01/22] util/coroutine: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../util/qemu-coroutine.c:150:8: error: ‘batch’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/qemu-coroutine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c
index eb4eebefdf..64d6264fc7 100644
--- a/util/qemu-coroutine.c
+++ b/util/qemu-coroutine.c
@@ -136,7 +136,7 @@ static Coroutine *coroutine_pool_get_local(void)
 static void coroutine_pool_refill_local(void)
 {
     CoroutinePool *local_pool = get_ptr_local_pool();
-    CoroutinePoolBatch *batch;
+    CoroutinePoolBatch *batch = NULL;
 
     WITH_QEMU_LOCK_GUARD(&global_pool_lock) {
         batch = QSLIST_FIRST(&global_pool);
-- 
2.45.2.827.g557ae147e6

[PATCH v3 02/22] util/timer: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../util/qemu-timer.c:198:24: error: ‘expire_time’ may be used uninitialized [-Werror=maybe-uninitialized]
../util/qemu-timer.c:476:8: error: ‘rearm’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 util/qemu-timer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index 6b1533bc2a..d5e33490fc 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -182,7 +182,7 @@ bool qemu_clock_has_timers(QEMUClockType type)
 
 bool timerlist_expired(QEMUTimerList *timer_list)
 {
-    int64_t expire_time;
+    int64_t expire_time = 0;
 
     if (!qatomic_read(&timer_list->active_timers)) {
         return false;
@@ -212,7 +212,7 @@ bool qemu_clock_expired(QEMUClockType type)
 int64_t timerlist_deadline_ns(QEMUTimerList *timer_list)
 {
     int64_t delta;
-    int64_t expire_time;
+    int64_t expire_time = 0;
 
     if (!qatomic_read(&timer_list->active_timers)) {
         return -1;
@@ -461,7 +461,7 @@ void timer_mod_ns(QEMUTimer *ts, int64_t expire_time)
 void timer_mod_anticipate_ns(QEMUTimer *ts, int64_t expire_time)
 {
     QEMUTimerList *timer_list = ts->timer_list;
-    bool rearm;
+    bool rearm = false;
 
     WITH_QEMU_LOCK_GUARD(&timer_list->active_timers_lock) {
         if (ts->expire_time == -1 || ts->expire_time > expire_time) {
-- 
2.45.2.827.g557ae147e6

[PATCH v3 03/22] hw/qxl: fix -Werror=maybe-uninitialized false-positives

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../hw/display/qxl.c:1352:5: error: ‘pci_region’ may be used uninitialized [-Werror=maybe-uninitialized]
../hw/display/qxl.c:1365:22: error: ‘pci_start’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 hw/display/qxl.c | 4 ++--
 roms/openbios    | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 3c2b5182ca..0c4b1c9bf2 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1301,8 +1301,8 @@ static int qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta,
     };
     uint64_t guest_start;
     uint64_t guest_end;
-    int pci_region;
-    pcibus_t pci_start;
+    int pci_region = -1;
+    pcibus_t pci_start = PCI_BAR_UNMAPPED;
     pcibus_t pci_end;
     MemoryRegion *mr;
     intptr_t virt_start;
diff --git a/roms/openbios b/roms/openbios
index c3a19c1e54..af97fd7af5 160000
--- a/roms/openbios
+++ b/roms/openbios
@@ -1 +1 @@
-Subproject commit c3a19c1e54977a53027d6232050e1e3e39a98a1b
+Subproject commit af97fd7af5e7c18f591a7b987291d3db4ffb28b5
-- 
2.45.2.827.g557ae147e6

[PATCH v3 04/22] nbd: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../nbd/client-connection.c:419:8: error: ‘wait_co’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 nbd/client-connection.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nbd/client-connection.c b/nbd/client-connection.c
index f9da67c87e..b11e266807 100644
--- a/nbd/client-connection.c
+++ b/nbd/client-connection.c
@@ -410,7 +410,7 @@ nbd_co_establish_connection(NBDClientConnection *conn, NBDExportInfo *info,
  */
 void nbd_co_establish_connection_cancel(NBDClientConnection *conn)
 {
-    Coroutine *wait_co;
+    Coroutine *wait_co = NULL;
 
     WITH_QEMU_LOCK_GUARD(&conn->mutex) {
         wait_co = g_steal_pointer(&conn->wait_co);
-- 
2.45.2.827.g557ae147e6

[PATCH v3 05/22] block/mirror: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../block/mirror.c:1066:22: error: ‘iostatus’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/mirror.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index 61f0a717b7..54e3a7ea9d 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -931,7 +931,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
     MirrorBDSOpaque *mirror_top_opaque = s->mirror_top_bs->opaque;
     BlockDriverState *target_bs = blk_bs(s->target);
     bool need_drain = true;
-    BlockDeviceIoStatus iostatus;
+    BlockDeviceIoStatus iostatus = BLOCK_DEVICE_IO_STATUS__MAX;
     int64_t length;
     int64_t target_length;
     BlockDriverInfo bdi;
-- 
2.45.2.827.g557ae147e6

[PATCH v3 06/22] block/mirror: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../block/mirror.c:404:5: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]
../block/mirror.c:895:12: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]
../block/mirror.c:578:12: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]

Change a variable to int, as suggested by Manos: "bdrv_co_preadv()
which is int and is passed as an int argument to mirror_read_complete()"

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 block/mirror.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 54e3a7ea9d..2afe700b4d 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -349,7 +349,7 @@ static void coroutine_fn mirror_co_read(void *opaque)
     MirrorOp *op = opaque;
     MirrorBlockJob *s = op->s;
     int nb_chunks;
-    uint64_t ret;
+    int ret = -1;
     uint64_t max_bytes;
 
     max_bytes = s->granularity * s->max_iov;
@@ -565,7 +565,7 @@ static void coroutine_fn GRAPH_UNLOCKED mirror_iteration(MirrorBlockJob *s)
 
     bitmap_set(s->in_flight_bitmap, offset / s->granularity, nb_chunks);
     while (nb_chunks > 0 && offset < s->bdev_length) {
-        int ret;
+        int ret = -1;
         int64_t io_bytes;
         int64_t io_bytes_acct;
         MirrorMethod mirror_method = MIRROR_METHOD_COPY;
@@ -841,7 +841,7 @@ static int coroutine_fn GRAPH_UNLOCKED mirror_dirty_init(MirrorBlockJob *s)
     int64_t offset;
     BlockDriverState *bs;
     BlockDriverState *target_bs = blk_bs(s->target);
-    int ret;
+    int ret = -1;
     int64_t count;
 
     bdrv_graph_co_rdlock();
-- 
2.45.2.827.g557ae147e6

Re: [PATCH v3 06/22] block/mirror: fix -Werror=maybe-uninitialized false-positive

[Vladimir Sementsov-Ogievskiy]

On 30.09.24 11:14, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau<marcandre.lureau@redhat.com>
> 
> ../block/mirror.c:404:5: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]
> ../block/mirror.c:895:12: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]
> ../block/mirror.c:578:12: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]
> 
> Change a variable to int, as suggested by Manos: "bdrv_co_preadv()
> which is int and is passed as an int argument to mirror_read_complete()"
> 
> Signed-off-by: Marc-André Lureau<marcandre.lureau@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

[PATCH v3 07/22] block/stream: fix -Werror=maybe-uninitialized false-positives

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../block/stream.c:193:19: error: ‘unfiltered_bs’ may be used uninitialized [-Werror=maybe-uninitialized]
../block/stream.c:176:5: error: ‘len’ may be used uninitialized [-Werror=maybe-uninitialized]
trace/trace-block.h:906:9: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 block/stream.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index 7031eef12b..9076203193 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -155,8 +155,8 @@ static void stream_clean(Job *job)
 static int coroutine_fn stream_run(Job *job, Error **errp)
 {
     StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
-    BlockDriverState *unfiltered_bs;
-    int64_t len;
+    BlockDriverState *unfiltered_bs = NULL;
+    int64_t len = -1;
     int64_t offset = 0;
     int error = 0;
     int64_t n = 0; /* bytes */
@@ -177,7 +177,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
 
     for ( ; offset < len; offset += n) {
         bool copy;
-        int ret;
+        int ret = -1;
 
         /* Note that even when no rate limit is applied we need to yield
          * with no pending I/O here so that bdrv_drain_all() returns.
-- 
2.45.2.827.g557ae147e6

[PATCH v3 08/22] hw/ahci: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../hw/ide/ahci.c:989:58: error: ‘tbl_entry_size’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 hw/ide/ahci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 7fc2a08df2..0eb24304ee 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -948,7 +948,6 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist,
     uint64_t sum = 0;
     int off_idx = -1;
     int64_t off_pos = -1;
-    int tbl_entry_size;
     IDEBus *bus = &ad->port;
     BusState *qbus = BUS(bus);
 
@@ -976,6 +975,8 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist,
     /* Get entries in the PRDT, init a qemu sglist accordingly */
     if (prdtl > 0) {
         AHCI_SG *tbl = (AHCI_SG *)prdt;
+        int tbl_entry_size = 0;
+
         sum = 0;
         for (i = 0; i < prdtl; i++) {
             tbl_entry_size = prdt_tbl_entry_size(&tbl[i]);
-- 
2.45.2.827.g557ae147e6

[PATCH v3 09/22] hw/vhost-scsi: fix -Werror=maybe-uninitialized

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../hw/scsi/vhost-scsi.c:173:12: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]

It can be reached when num_queues=0. It probably doesn't make much sense
to instantiate a vhost-scsi with 0 IO queues though. For now, make
vhost_scsi_set_workers() return success/0 anyway, when no workers have
been setup.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
---
 hw/scsi/vhost-scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 49cff2a0cb..22d16dc26b 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -172,7 +172,7 @@ static int vhost_scsi_set_workers(VHostSCSICommon *vsc, bool per_virtqueue)
     struct vhost_dev *dev = &vsc->dev;
     struct vhost_vring_worker vq_worker;
     struct vhost_worker_state worker;
-    int i, ret;
+    int i, ret = 0;
 
     /* Use default worker */
     if (!per_virtqueue || dev->nvqs == VHOST_SCSI_VQ_NUM_FIXED + 1) {
-- 
2.45.2.827.g557ae147e6

[PATCH v3 10/22] hw/sdhci: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../hw/sd/sdhci.c:846:16: error: ‘res’ may be used uninitialized [-Werror=maybe-uninitialized]

False-positive, because "length" is non-null.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/sd/sdhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 87122e4245..ed01499391 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -747,7 +747,7 @@ static void sdhci_do_adma(SDHCIState *s)
     const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;
     const MemTxAttrs attrs = { .memory = true };
     ADMADescr dscr = {};
-    MemTxResult res;
+    MemTxResult res = MEMTX_ERROR;
     int i;
 
     if (s->trnmod & SDHC_TRNS_BLK_CNT_EN && !s->blkcnt) {
-- 
2.45.2.827.g557ae147e6

Re: [PATCH v3 10/22] hw/sdhci: fix -Werror=maybe-uninitialized false-positive

[Alex Bennée]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> ../hw/sd/sdhci.c:846:16: error: ‘res’ may be used uninitialized [-Werror=maybe-uninitialized]
>
> False-positive, because "length" is non-null.

I certainly get that:

  length = dscr.length ? dscr.length : 64 * KiB;

means we always have something. Although get_adma_description() is
deserving of a g_assert_not_reached() lest we end up re-using a previous
descr.

I guess wider re-factoring is out of scope for this series though:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH v3 11/22] block/block-copy: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../block/block-copy.c:591:12: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 block/block-copy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index cc618e4561..7265733825 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -568,7 +568,7 @@ static coroutine_fn int block_copy_task_entry(AioTask *task)
     BlockCopyState *s = t->s;
     bool error_is_read = false;
     BlockCopyMethod method = t->method;
-    int ret;
+    int ret = -1;
 
     WITH_GRAPH_RDLOCK_GUARD() {
         ret = block_copy_do_copy(s, t->req.offset, t->req.bytes, &method,
-- 
2.45.2.827.g557ae147e6

Re: [PATCH v3 11/22] block/block-copy: fix -Werror=maybe-uninitialized false-positive

[Vladimir Sementsov-Ogievskiy]

On 30.09.24 11:14, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau<marcandre.lureau@redhat.com>
> 
> ../block/block-copy.c:591:12: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]
> 
> Signed-off-by: Marc-André Lureau<marcandre.lureau@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

[PATCH v3 12/22] migration: fix -Werror=maybe-uninitialized false-positives

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../migration/dirtyrate.c:186:5: error: ‘records’ may be used uninitialized [-Werror=maybe-uninitialized]
../migration/dirtyrate.c:168:12: error: ‘gen_id’ may be used uninitialized [-Werror=maybe-uninitialized]
../migration/migration.c:2273:5: error: ‘file’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Hyman Huang <yong.huang@smartx.com>
---
 migration/dirtyrate.c | 4 ++--
 migration/migration.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 5478d58de3..233acb0855 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -149,12 +149,12 @@ int64_t vcpu_calculate_dirtyrate(int64_t calc_time_ms,
                                  unsigned int flag,
                                  bool one_shot)
 {
-    DirtyPageRecord *records;
+    DirtyPageRecord *records = NULL;
     int64_t init_time_ms;
     int64_t duration;
     int64_t dirtyrate;
     int i = 0;
-    unsigned int gen_id;
+    unsigned int gen_id = 0;
 
 retry:
     init_time_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
diff --git a/migration/migration.c b/migration/migration.c
index ae2be31557..021faee2f3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2278,7 +2278,7 @@ static bool migrate_handle_rp_resume_ack(MigrationState *s,
  */
 static void migration_release_dst_files(MigrationState *ms)
 {
-    QEMUFile *file;
+    QEMUFile *file = NULL;
 
     WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
         /*
-- 
2.45.2.827.g557ae147e6

[PATCH v3 13/22] hw/virtio-blk: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../hw/block/virtio-blk.c:1212:12: error: ‘rq’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
---
 hw/block/virtio-blk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 115795392c..9166d7974d 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1060,7 +1060,7 @@ static void virtio_blk_dma_restart_cb(void *opaque, bool running,
     VirtIOBlock *s = opaque;
     uint16_t num_queues = s->conf.num_queues;
     g_autofree VirtIOBlockReq **vq_rq = NULL;
-    VirtIOBlockReq *rq;
+    VirtIOBlockReq *rq = NULL;
 
     if (!running) {
         return;
-- 
2.45.2.827.g557ae147e6

[PATCH v3 14/22] migration: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../migration/ram.c:1873:23: error: ‘dirty’ may be used uninitialized [-Werror=maybe-uninitialized]

When 'block' != NULL, 'dirty' is initialized.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Peter Xu <peterx@redhat.com>
---
 migration/ram.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index 81eda2736a..326ce7eb79 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1793,7 +1793,7 @@ static bool get_queued_page(RAMState *rs, PageSearchStatus *pss)
 {
     RAMBlock  *block;
     ram_addr_t offset;
-    bool dirty;
+    bool dirty = false;
 
     do {
         block = unqueue_page(rs, &offset);
-- 
2.45.2.827.g557ae147e6

[PATCH v3 15/22] linux-user/hppa: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../linux-user/hppa/cpu_loop.c: In function ‘hppa_lws’:
../linux-user/hppa/cpu_loop.c:106:17: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]
  106 |     env->gr[28] = ret;

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 linux-user/hppa/cpu_loop.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/linux-user/hppa/cpu_loop.c b/linux-user/hppa/cpu_loop.c
index bc093b8fe8..f4da95490e 100644
--- a/linux-user/hppa/cpu_loop.c
+++ b/linux-user/hppa/cpu_loop.c
@@ -43,7 +43,7 @@ static abi_ulong hppa_lws(CPUHPPAState *env)
         old = tswap32(old);
         new = tswap32(new);
         ret = qatomic_cmpxchg((uint32_t *)g2h(cs, addr), old, new);
-        ret = tswap32(ret);
+        env->gr[28] = tswap32(ret);
         break;
 
     case 2: /* elf32 atomic "new" cmpxchg */
@@ -64,19 +64,19 @@ static abi_ulong hppa_lws(CPUHPPAState *env)
             old = *(uint8_t *)g2h(cs, old);
             new = *(uint8_t *)g2h(cs, new);
             ret = qatomic_cmpxchg((uint8_t *)g2h(cs, addr), old, new);
-            ret = ret != old;
+            env->gr[28] = ret != old;
             break;
         case 1:
             old = *(uint16_t *)g2h(cs, old);
             new = *(uint16_t *)g2h(cs, new);
             ret = qatomic_cmpxchg((uint16_t *)g2h(cs, addr), old, new);
-            ret = ret != old;
+            env->gr[28] = ret != old;
             break;
         case 2:
             old = *(uint32_t *)g2h(cs, old);
             new = *(uint32_t *)g2h(cs, new);
             ret = qatomic_cmpxchg((uint32_t *)g2h(cs, addr), old, new);
-            ret = ret != old;
+            env->gr[28] = ret != old;
             break;
         case 3:
             {
@@ -97,13 +97,13 @@ static abi_ulong hppa_lws(CPUHPPAState *env)
                 }
                 end_exclusive();
 #endif
+                env->gr[28] = ret;
             }
             break;
         }
         break;
     }
 
-    env->gr[28] = ret;
     return 0;
 }
 
-- 
2.45.2.827.g557ae147e6

Re: [PATCH v3 15/22] linux-user/hppa: fix -Werror=maybe-uninitialized false-positive

[Laurent Vivier]

CC Helge Deller

Le 30/09/2024 à 10:14, marcandre.lureau@redhat.com a écrit :
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> ../linux-user/hppa/cpu_loop.c: In function ‘hppa_lws’:
> ../linux-user/hppa/cpu_loop.c:106:17: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]
>    106 |     env->gr[28] = ret;
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   linux-user/hppa/cpu_loop.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/linux-user/hppa/cpu_loop.c b/linux-user/hppa/cpu_loop.c
> index bc093b8fe8..f4da95490e 100644
> --- a/linux-user/hppa/cpu_loop.c
> +++ b/linux-user/hppa/cpu_loop.c
> @@ -43,7 +43,7 @@ static abi_ulong hppa_lws(CPUHPPAState *env)
>           old = tswap32(old);
>           new = tswap32(new);
>           ret = qatomic_cmpxchg((uint32_t *)g2h(cs, addr), old, new);
> -        ret = tswap32(ret);
> +        env->gr[28] = tswap32(ret);
>           break;
>   
>       case 2: /* elf32 atomic "new" cmpxchg */
> @@ -64,19 +64,19 @@ static abi_ulong hppa_lws(CPUHPPAState *env)
>               old = *(uint8_t *)g2h(cs, old);
>               new = *(uint8_t *)g2h(cs, new);
>               ret = qatomic_cmpxchg((uint8_t *)g2h(cs, addr), old, new);
> -            ret = ret != old;
> +            env->gr[28] = ret != old;
>               break;
>           case 1:
>               old = *(uint16_t *)g2h(cs, old);
>               new = *(uint16_t *)g2h(cs, new);
>               ret = qatomic_cmpxchg((uint16_t *)g2h(cs, addr), old, new);
> -            ret = ret != old;
> +            env->gr[28] = ret != old;
>               break;
>           case 2:
>               old = *(uint32_t *)g2h(cs, old);
>               new = *(uint32_t *)g2h(cs, new);
>               ret = qatomic_cmpxchg((uint32_t *)g2h(cs, addr), old, new);
> -            ret = ret != old;
> +            env->gr[28] = ret != old;
>               break;
>           case 3:
>               {
> @@ -97,13 +97,13 @@ static abi_ulong hppa_lws(CPUHPPAState *env)
>                   }
>                   end_exclusive();
>   #endif
> +                env->gr[28] = ret;
>               }
>               break;
>           }
>           break;
>       }
>   
> -    env->gr[28] = ret;
>       return 0;
>   }
>   

Did you try to put a g_assert_not_reached() in a "default:" for "switch(size)"?
This should help the compiler to know that "env->gr[28] = ret;" will not be reached if ret is not set.

Thanks,
Laurent

Re: [PATCH v3 15/22] linux-user/hppa: fix -Werror=maybe-uninitialized false-positive

[Marc-André Lureau]

Hi Laurent

On Mon, Sep 30, 2024 at 4:19 PM Laurent Vivier <laurent@vivier.eu> wrote:
>
> CC Helge Deller
>
> Le 30/09/2024 à 10:14, marcandre.lureau@redhat.com a écrit :
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > ../linux-user/hppa/cpu_loop.c: In function ‘hppa_lws’:
> > ../linux-user/hppa/cpu_loop.c:106:17: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]
> >    106 |     env->gr[28] = ret;
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   linux-user/hppa/cpu_loop.c | 10 +++++-----
> >   1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/linux-user/hppa/cpu_loop.c b/linux-user/hppa/cpu_loop.c
> > index bc093b8fe8..f4da95490e 100644
> > --- a/linux-user/hppa/cpu_loop.c
> > +++ b/linux-user/hppa/cpu_loop.c
> > @@ -43,7 +43,7 @@ static abi_ulong hppa_lws(CPUHPPAState *env)
> >           old = tswap32(old);
> >           new = tswap32(new);
> >           ret = qatomic_cmpxchg((uint32_t *)g2h(cs, addr), old, new);
> > -        ret = tswap32(ret);
> > +        env->gr[28] = tswap32(ret);
> >           break;
> >
> >       case 2: /* elf32 atomic "new" cmpxchg */
> > @@ -64,19 +64,19 @@ static abi_ulong hppa_lws(CPUHPPAState *env)
> >               old = *(uint8_t *)g2h(cs, old);
> >               new = *(uint8_t *)g2h(cs, new);
> >               ret = qatomic_cmpxchg((uint8_t *)g2h(cs, addr), old, new);
> > -            ret = ret != old;
> > +            env->gr[28] = ret != old;
> >               break;
> >           case 1:
> >               old = *(uint16_t *)g2h(cs, old);
> >               new = *(uint16_t *)g2h(cs, new);
> >               ret = qatomic_cmpxchg((uint16_t *)g2h(cs, addr), old, new);
> > -            ret = ret != old;
> > +            env->gr[28] = ret != old;
> >               break;
> >           case 2:
> >               old = *(uint32_t *)g2h(cs, old);
> >               new = *(uint32_t *)g2h(cs, new);
> >               ret = qatomic_cmpxchg((uint32_t *)g2h(cs, addr), old, new);
> > -            ret = ret != old;
> > +            env->gr[28] = ret != old;
> >               break;
> >           case 3:
> >               {
> > @@ -97,13 +97,13 @@ static abi_ulong hppa_lws(CPUHPPAState *env)
> >                   }
> >                   end_exclusive();
> >   #endif
> > +                env->gr[28] = ret;
> >               }
> >               break;
> >           }
> >           break;
> >       }
> >
> > -    env->gr[28] = ret;
> >       return 0;
> >   }
> >
>
> Did you try to put a g_assert_not_reached() in a "default:" for "switch(size)"?
> This should help the compiler to know that "env->gr[28] = ret;" will not be reached if ret is not set.

That works! I'll change the patch and include your r-b then?

Re: [PATCH v3 15/22] linux-user/hppa: fix -Werror=maybe-uninitialized false-positive

[Laurent Vivier]

Le 30/09/2024 à 15:26, Marc-André Lureau a écrit :
> Hi Laurent
> 
> On Mon, Sep 30, 2024 at 4:19 PM Laurent Vivier <laurent@vivier.eu> wrote:
>>
>> CC Helge Deller
>>
>> Le 30/09/2024 à 10:14, marcandre.lureau@redhat.com a écrit :
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> ../linux-user/hppa/cpu_loop.c: In function ‘hppa_lws’:
>>> ../linux-user/hppa/cpu_loop.c:106:17: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]
>>>     106 |     env->gr[28] = ret;
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>    linux-user/hppa/cpu_loop.c | 10 +++++-----
>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/linux-user/hppa/cpu_loop.c b/linux-user/hppa/cpu_loop.c
>>> index bc093b8fe8..f4da95490e 100644
>>> --- a/linux-user/hppa/cpu_loop.c
>>> +++ b/linux-user/hppa/cpu_loop.c
>>> @@ -43,7 +43,7 @@ static abi_ulong hppa_lws(CPUHPPAState *env)
>>>            old = tswap32(old);
>>>            new = tswap32(new);
>>>            ret = qatomic_cmpxchg((uint32_t *)g2h(cs, addr), old, new);
>>> -        ret = tswap32(ret);
>>> +        env->gr[28] = tswap32(ret);
>>>            break;
>>>
>>>        case 2: /* elf32 atomic "new" cmpxchg */
>>> @@ -64,19 +64,19 @@ static abi_ulong hppa_lws(CPUHPPAState *env)
>>>                old = *(uint8_t *)g2h(cs, old);
>>>                new = *(uint8_t *)g2h(cs, new);
>>>                ret = qatomic_cmpxchg((uint8_t *)g2h(cs, addr), old, new);
>>> -            ret = ret != old;
>>> +            env->gr[28] = ret != old;
>>>                break;
>>>            case 1:
>>>                old = *(uint16_t *)g2h(cs, old);
>>>                new = *(uint16_t *)g2h(cs, new);
>>>                ret = qatomic_cmpxchg((uint16_t *)g2h(cs, addr), old, new);
>>> -            ret = ret != old;
>>> +            env->gr[28] = ret != old;
>>>                break;
>>>            case 2:
>>>                old = *(uint32_t *)g2h(cs, old);
>>>                new = *(uint32_t *)g2h(cs, new);
>>>                ret = qatomic_cmpxchg((uint32_t *)g2h(cs, addr), old, new);
>>> -            ret = ret != old;
>>> +            env->gr[28] = ret != old;
>>>                break;
>>>            case 3:
>>>                {
>>> @@ -97,13 +97,13 @@ static abi_ulong hppa_lws(CPUHPPAState *env)
>>>                    }
>>>                    end_exclusive();
>>>    #endif
>>> +                env->gr[28] = ret;
>>>                }
>>>                break;
>>>            }
>>>            break;
>>>        }
>>>
>>> -    env->gr[28] = ret;
>>>        return 0;
>>>    }
>>>
>>
>> Did you try to put a g_assert_not_reached() in a "default:" for "switch(size)"?
>> This should help the compiler to know that "env->gr[28] = ret;" will not be reached if ret is not set.
> 
> That works! I'll change the patch and include your r-b then?
> 
> 

Yes, you can

Thanks,
Laurent

[PATCH v3 16/22] target/loongarch: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../target/loongarch/gdbstub.c:55:20: error: ‘val’ may be used uninitialized [-Werror=maybe-uninitialized]
   55 |             return gdb_get_reg32(mem_buf, val);
      |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../target/loongarch/gdbstub.c:39:18: note: ‘val’ was declared here
   39 |         uint64_t val;

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 target/loongarch/gdbstub.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/target/loongarch/gdbstub.c b/target/loongarch/gdbstub.c
index 7ca245ee81..3a03cf9cba 100644
--- a/target/loongarch/gdbstub.c
+++ b/target/loongarch/gdbstub.c
@@ -34,26 +34,28 @@ void write_fcc(CPULoongArchState *env, uint64_t val)
 int loongarch_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
 {
     CPULoongArchState *env = cpu_env(cs);
-    uint64_t val;
-
-    if (0 <= n && n < 32) {
-        val = env->gpr[n];
-    } else if (n == 32) {
-        /* orig_a0 */
-        val = 0;
-    } else if (n == 33) {
-        val = env->pc;
-    } else if (n == 34) {
-        val = env->CSR_BADV;
-    }
 
     if (0 <= n && n <= 34) {
+        uint64_t val;
+
+        if (n < 32) {
+            val = env->gpr[n];
+        } else if (n == 32) {
+            /* orig_a0 */
+            val = 0;
+        } else if (n == 33) {
+            val = env->pc;
+        } else /* if (n == 34) */ {
+            val = env->CSR_BADV;
+        }
+
         if (is_la64(env)) {
             return gdb_get_reg64(mem_buf, val);
         } else {
             return gdb_get_reg32(mem_buf, val);
         }
     }
+
     return 0;
 }
 
-- 
2.45.2.827.g557ae147e6

Re: [PATCH v3 16/22] target/loongarch: fix -Werror=maybe-uninitialized false-positive

[Vladimir Sementsov-Ogievskiy]

On 30.09.24 11:14, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau<marcandre.lureau@redhat.com>
> 
> ../target/loongarch/gdbstub.c:55:20: error: ‘val’ may be used uninitialized [-Werror=maybe-uninitialized]
>     55 |             return gdb_get_reg32(mem_buf, val);
>        |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../target/loongarch/gdbstub.c:39:18: note: ‘val’ was declared here
>     39 |         uint64_t val;
> 
> Signed-off-by: Marc-André Lureau<marcandre.lureau@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

[PATCH v3 17/22] tests: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../tests/unit/test-block-iothread.c:773:17: error: ‘job’ may be used uninitialized [-Werror=maybe-uninitialized]
/usr/include/glib-2.0/glib/gtestutils.h:73:53: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/unit/test-bdrv-drain.c     | 2 +-
 tests/unit/test-block-iothread.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 666880472b..c112d5b189 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -722,7 +722,7 @@ static void test_blockjob_common_drain_node(enum drain_type drain_type,
     BlockJob *job;
     TestBlockJob *tjob;
     IOThread *iothread = NULL;
-    int ret;
+    int ret = -1;
 
     src = bdrv_new_open_driver(&bdrv_test, "source", BDRV_O_RDWR,
                                &error_abort);
diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c
index 3766d5de6b..20ed54f570 100644
--- a/tests/unit/test-block-iothread.c
+++ b/tests/unit/test-block-iothread.c
@@ -745,7 +745,7 @@ static void test_propagate_mirror(void)
     AioContext *main_ctx = qemu_get_aio_context();
     BlockDriverState *src, *target, *filter;
     BlockBackend *blk;
-    Job *job;
+    Job *job = NULL;
     Error *local_err = NULL;
 
     /* Create src and target*/
-- 
2.45.2.827.g557ae147e6

Re: [PATCH v3 17/22] tests: fix -Werror=maybe-uninitialized false-positive

[Vladimir Sementsov-Ogievskiy]

On 30.09.24 11:14, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau<marcandre.lureau@redhat.com>
> 
> ../tests/unit/test-block-iothread.c:773:17: error: ‘job’ may be used uninitialized [-Werror=maybe-uninitialized]
> /usr/include/glib-2.0/glib/gtestutils.h:73:53: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]
> 
> Signed-off-by: Marc-André Lureau<marcandre.lureau@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

[PATCH v3 18/22] hw/virtio: fix -Werror=maybe-uninitialized

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../hw/virtio/vhost-shadow-virtqueue.c:545:13: error: ‘r’ may be used uninitialized [-Werror=maybe-uninitialized]

Set `r` to 0 at every loop, since we don't check vhost_svq_get_buf()
return value.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost-shadow-virtqueue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
index fc5f408f77..3b2beaea24 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -526,10 +526,10 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
 size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
 {
     size_t len = 0;
-    uint32_t r;
 
     while (num--) {
         int64_t start_us = g_get_monotonic_time();
+        uint32_t r = 0;
 
         do {
             if (vhost_svq_more_used(svq)) {
-- 
2.45.2.827.g557ae147e6

Re: [PATCH v3 18/22] hw/virtio: fix -Werror=maybe-uninitialized

[Stefano Garzarella]

On Mon, Sep 30, 2024 at 12:14:53PM GMT, marcandre.lureau@redhat.com wrote:
>From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>../hw/virtio/vhost-shadow-virtqueue.c:545:13: error: ‘r’ may be used uninitialized [-Werror=maybe-uninitialized]
>
>Set `r` to 0 at every loop, since we don't check vhost_svq_get_buf()
>return value.
>
>Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>---
> hw/virtio/vhost-shadow-virtqueue.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>
>diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
>index fc5f408f77..3b2beaea24 100644
>--- a/hw/virtio/vhost-shadow-virtqueue.c
>+++ b/hw/virtio/vhost-shadow-virtqueue.c
>@@ -526,10 +526,10 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
> size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
> {
>     size_t len = 0;
>-    uint32_t r;
>
>     while (num--) {
>         int64_t start_us = g_get_monotonic_time();
>+        uint32_t r = 0;
>
>         do {
>             if (vhost_svq_more_used(svq)) {
>-- 
>2.45.2.827.g557ae147e6
>

[PATCH v3 19/22] RFC: hw/virtio: a potential leak fix

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

vhost_svq_get_buf() may return a VirtQueueElement that should be freed.

It's unclear to me if the vhost_svq_get_buf() call should always return NULL.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost-shadow-virtqueue.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
index 3b2beaea24..37aca8b431 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -414,6 +414,7 @@ static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
     return i;
 }
 
+G_GNUC_WARN_UNUSED_RESULT
 static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
                                            uint32_t *len)
 {
@@ -528,6 +529,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
     size_t len = 0;
 
     while (num--) {
+        g_autofree VirtQueueElement *elem = NULL;
         int64_t start_us = g_get_monotonic_time();
         uint32_t r = 0;
 
@@ -541,7 +543,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
             }
         } while (true);
 
-        vhost_svq_get_buf(svq, &r);
+        elem = vhost_svq_get_buf(svq, &r);
         len += r;
     }
 
-- 
2.45.2.827.g557ae147e6

Re: [PATCH v3 19/22] RFC: hw/virtio: a potential leak fix

[Eugenio Perez Martin]

On Mon, Sep 30, 2024 at 10:17 AM <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> vhost_svq_get_buf() may return a VirtQueueElement that should be freed.
>
> It's unclear to me if the vhost_svq_get_buf() call should always return NULL.
>

Continuing conversation of v2,

Yes there are situations where vhost_svq_get_buf can return a valid
buffer here and we could leak memory, so this fixes a bug.

So,

Reviewed-by: Eugenio Pérez <eperezma@redhat.com>

Thanks!

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/virtio/vhost-shadow-virtqueue.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> index 3b2beaea24..37aca8b431 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -414,6 +414,7 @@ static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
>      return i;
>  }
>
> +G_GNUC_WARN_UNUSED_RESULT
>  static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
>                                             uint32_t *len)
>  {
> @@ -528,6 +529,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
>      size_t len = 0;
>
>      while (num--) {
> +        g_autofree VirtQueueElement *elem = NULL;
>          int64_t start_us = g_get_monotonic_time();
>          uint32_t r = 0;
>
> @@ -541,7 +543,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
>              }
>          } while (true);
>
> -        vhost_svq_get_buf(svq, &r);
> +        elem = vhost_svq_get_buf(svq, &r);
>          len += r;
>      }
>
> --
> 2.45.2.827.g557ae147e6
>

Re: [PATCH v3 19/22] RFC: hw/virtio: a potential leak fix

[Eugenio Perez Martin]

On Mon, Sep 30, 2024 at 1:02 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Mon, Sep 30, 2024 at 10:17 AM <marcandre.lureau@redhat.com> wrote:
> >
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > vhost_svq_get_buf() may return a VirtQueueElement that should be freed.
> >
> > It's unclear to me if the vhost_svq_get_buf() call should always return NULL.
> >
>
> Continuing conversation of v2,
>
> Yes there are situations where vhost_svq_get_buf can return a valid
> buffer here and we could leak memory, so this fixes a bug.
>
> So,
>
> Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
>
> Thanks!
>

(I hit "send" too early)

Wwe could use a better patch subject though. Even "Freeing leaked
memory from vhost_svq_get_buf in vhost_svq_poll" would work better for
me. What do you think?

Thanks!

> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  hw/virtio/vhost-shadow-virtqueue.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> > index 3b2beaea24..37aca8b431 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > @@ -414,6 +414,7 @@ static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
> >      return i;
> >  }
> >
> > +G_GNUC_WARN_UNUSED_RESULT
> >  static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
> >                                             uint32_t *len)
> >  {
> > @@ -528,6 +529,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
> >      size_t len = 0;
> >
> >      while (num--) {
> > +        g_autofree VirtQueueElement *elem = NULL;
> >          int64_t start_us = g_get_monotonic_time();
> >          uint32_t r = 0;
> >
> > @@ -541,7 +543,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
> >              }
> >          } while (true);
> >
> > -        vhost_svq_get_buf(svq, &r);
> > +        elem = vhost_svq_get_buf(svq, &r);
> >          len += r;
> >      }
> >
> > --
> > 2.45.2.827.g557ae147e6
> >

Re: [PATCH v3 19/22] RFC: hw/virtio: a potential leak fix

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2490 bytes --]

On Mon, Sep 30, 2024 at 3:05 PM Eugenio Perez Martin <eperezma@redhat.com>
wrote:

> On Mon, Sep 30, 2024 at 1:02 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Mon, Sep 30, 2024 at 10:17 AM <marcandre.lureau@redhat.com> wrote:
> > >
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > vhost_svq_get_buf() may return a VirtQueueElement that should be freed.
> > >
> > > It's unclear to me if the vhost_svq_get_buf() call should always
> return NULL.
> > >
> >
> > Continuing conversation of v2,
> >
> > Yes there are situations where vhost_svq_get_buf can return a valid
> > buffer here and we could leak memory, so this fixes a bug.
> >
> > So,
> >
> > Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
> >
> > Thanks!
> >
>
> (I hit "send" too early)
>
> Wwe could use a better patch subject though. Even "Freeing leaked
> memory from vhost_svq_get_buf in vhost_svq_poll" would work better for
> me. What do you think?
>
> Thanks!
>

ok thanks!


>
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > >  hw/virtio/vhost-shadow-virtqueue.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c
> b/hw/virtio/vhost-shadow-virtqueue.c
> > > index 3b2beaea24..37aca8b431 100644
> > > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > > @@ -414,6 +414,7 @@ static uint16_t vhost_svq_last_desc_of_chain(const
> VhostShadowVirtqueue *svq,
> > >      return i;
> > >  }
> > >
> > > +G_GNUC_WARN_UNUSED_RESULT
> > >  static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
> > >                                             uint32_t *len)
> > >  {
> > > @@ -528,6 +529,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq,
> size_t num)
> > >      size_t len = 0;
> > >
> > >      while (num--) {
> > > +        g_autofree VirtQueueElement *elem = NULL;
> > >          int64_t start_us = g_get_monotonic_time();
> > >          uint32_t r = 0;
> > >
> > > @@ -541,7 +543,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq,
> size_t num)
> > >              }
> > >          } while (true);
> > >
> > > -        vhost_svq_get_buf(svq, &r);
> > > +        elem = vhost_svq_get_buf(svq, &r);
> > >          len += r;
> > >      }
> > >
> > > --
> > > 2.45.2.827.g557ae147e6
> > >
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3956 bytes --]

[PATCH v3 20/22] block: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../block/file-posix.c:1405:17: error: ‘zoned’ may be used uninitialized [-Werror=maybe-uninitialized]
 1405 |     if (ret < 0 || zoned == BLK_Z_NONE) {

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 block/file-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index ff928b5e85..90fa54352c 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1398,7 +1398,7 @@ static void raw_refresh_zoned_limits(BlockDriverState *bs, struct stat *st,
                                      Error **errp)
 {
     BDRVRawState *s = bs->opaque;
-    BlockZoneModel zoned;
+    BlockZoneModel zoned = BLK_Z_NONE;
     int ret;
 
     ret = get_sysfs_zoned_model(st, &zoned);
-- 
2.45.2.827.g557ae147e6

Re: [PATCH v3 20/22] block: fix -Werror=maybe-uninitialized false-positive

[Vladimir Sementsov-Ogievskiy]

On 30.09.24 11:14, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau<marcandre.lureau@redhat.com>
> 
> ../block/file-posix.c:1405:17: error: ‘zoned’ may be used uninitialized [-Werror=maybe-uninitialized]
>   1405 |     if (ret < 0 || zoned == BLK_Z_NONE) {
> 
> Signed-off-by: Marc-André Lureau<marcandre.lureau@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>


-- 
Best regards,
Vladimir

Re: [PATCH v3 20/22] block: fix -Werror=maybe-uninitialized false-positive

[Kevin Wolf]

Am 30.09.2024 um 10:14 hat marcandre.lureau@redhat.com geschrieben:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> ../block/file-posix.c:1405:17: error: ‘zoned’ may be used uninitialized [-Werror=maybe-uninitialized]
>  1405 |     if (ret < 0 || zoned == BLK_Z_NONE) {
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[PATCH v3 21/22] qom/object: fix -Werror=maybe-uninitialized

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

object_resolve_path_type() didn't always set *ambiguousp.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qom/object.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/qom/object.c b/qom/object.c
index 28c5b66eab..bdc8a2c666 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -2201,6 +2201,9 @@ Object *object_resolve_path_type(const char *path, const char *typename,
         }
     } else {
         obj = object_resolve_abs_path(object_get_root(), parts + 1, typename);
+        if (ambiguousp) {
+            *ambiguousp = false;
+        }
     }
 
     g_strfreev(parts);
@@ -2226,7 +2229,7 @@ Object *object_resolve_path_at(Object *parent, const char *path)
 
 Object *object_resolve_type_unambiguous(const char *typename, Error **errp)
 {
-    bool ambig;
+    bool ambig = false;
     Object *o = object_resolve_path_type("", typename, &ambig);
 
     if (ambig) {
-- 
2.45.2.827.g557ae147e6

Re: [PATCH v3 21/22] qom/object: fix -Werror=maybe-uninitialized

[Vladimir Sementsov-Ogievskiy]

On 30.09.24 11:14, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> object_resolve_path_type() didn't always set *ambiguousp.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   qom/object.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/qom/object.c b/qom/object.c
> index 28c5b66eab..bdc8a2c666 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -2201,6 +2201,9 @@ Object *object_resolve_path_type(const char *path, const char *typename,
>           }
>       } else {
>           obj = object_resolve_abs_path(object_get_root(), parts + 1, typename);
> +        if (ambiguousp) {
> +            *ambiguousp = false;
> +        }

Doesn't this hunk in isolation fix the issue? With this object_resolve_path_type() should set the pointer on all paths if it is non-null..

Hmm, called object_resolve_partial_path() also doesn't set ambiguous on every path, so this hunk is at lease incomplete.

I'm unsure about what semantics expected around ambigous pointers, but it seems to me that it is set only on failure paths, as a reason, why we failed. If this is true, I think, we need only the second hunk, which initializes local "ambig".

>       }
>   
>       g_strfreev(parts);
> @@ -2226,7 +2229,7 @@ Object *object_resolve_path_at(Object *parent, const char *path)
>   
>   Object *object_resolve_type_unambiguous(const char *typename, Error **errp)
>   {
> -    bool ambig;
> +    bool ambig = false;
>       Object *o = object_resolve_path_type("", typename, &ambig);
>   
>       if (ambig) {

-- 
Best regards,
Vladimir

Re: [PATCH v3 21/22] qom/object: fix -Werror=maybe-uninitialized

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2702 bytes --]

Hi Vladimir

On Tue, Oct 1, 2024 at 6:06 PM Vladimir Sementsov-Ogievskiy <
vsementsov@yandex-team.ru> wrote:

> On 30.09.24 11:14, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > object_resolve_path_type() didn't always set *ambiguousp.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   qom/object.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/qom/object.c b/qom/object.c
> > index 28c5b66eab..bdc8a2c666 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -2201,6 +2201,9 @@ Object *object_resolve_path_type(const char *path,
> const char *typename,
> >           }
> >       } else {
> >           obj = object_resolve_abs_path(object_get_root(), parts + 1,
> typename);
> > +        if (ambiguousp) {
> > +            *ambiguousp = false;
> > +        }
>
> Doesn't this hunk in isolation fix the issue? With this
> object_resolve_path_type() should set the pointer on all paths if it is
> non-null..
>
>


> Hmm, called object_resolve_partial_path() also doesn't set ambiguous on
> every path, so this hunk is at lease incomplete.
>

yeah, but object_resolve_path_type() initializes it.

I'm unsure about what semantics expected around ambigous pointers, but it
> seems to me that it is set only on failure paths, as a reason, why we
> failed. If this is true, I think, we need only the second hunk, which
> initializes local "ambig".
>
>
right, and that seems good enough.

Do you ack/rb this change then?


    qom/object: fix -Werror=maybe-uninitialized

    object_resolve_path_type() didn't always set *ambiguousp.

    Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

diff --git a/qom/object.c b/qom/object.c
index 28c5b66eab..d3d3003541 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -2226,7 +2226,7 @@ Object *object_resolve_path_at(Object *parent, const
char *path)

 Object *object_resolve_type_unambiguous(const char *typename, Error **errp)
 {
-    bool ambig;
+    bool ambig = false;
     Object *o = object_resolve_path_type("", typename, &ambig);

     if (ambig) {


thanks!


> >       }
> >
> >       g_strfreev(parts);
> > @@ -2226,7 +2229,7 @@ Object *object_resolve_path_at(Object *parent,
> const char *path)
> >
> >   Object *object_resolve_type_unambiguous(const char *typename, Error
> **errp)
> >   {
> > -    bool ambig;
> > +    bool ambig = false;
> >       Object *o = object_resolve_path_type("", typename, &ambig);
> >
> >       if (ambig) {
>
> --
> Best regards,
> Vladimir
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 4350 bytes --]

Re: [PATCH v3 21/22] qom/object: fix -Werror=maybe-uninitialized

[Vladimir Sementsov-Ogievskiy]

On 01.10.24 18:22, Marc-André Lureau wrote:
> Hi Vladimir
> 
> On Tue, Oct 1, 2024 at 6:06 PM Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru <mailto:vsementsov@yandex-team.ru>> wrote:
> 
>     On 30.09.24 11:14, marcandre.lureau@redhat.com <mailto:marcandre.lureau@redhat.com> wrote:
>      > From: Marc-André Lureau <marcandre.lureau@redhat.com <mailto:marcandre.lureau@redhat.com>>
>      >
>      > object_resolve_path_type() didn't always set *ambiguousp.
>      >
>      > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com <mailto:marcandre.lureau@redhat.com>>
>      > ---
>      >   qom/object.c | 5 ++++-
>      >   1 file changed, 4 insertions(+), 1 deletion(-)
>      >
>      > diff --git a/qom/object.c b/qom/object.c
>      > index 28c5b66eab..bdc8a2c666 100644
>      > --- a/qom/object.c
>      > +++ b/qom/object.c
>      > @@ -2201,6 +2201,9 @@ Object *object_resolve_path_type(const char *path, const char *typename,
>      >           }
>      >       } else {
>      >           obj = object_resolve_abs_path(object_get_root(), parts + 1, typename);
>      > +        if (ambiguousp) {
>      > +            *ambiguousp = false;
>      > +        }
> 
>     Doesn't this hunk in isolation fix the issue? With this object_resolve_path_type() should set the pointer on all paths if it is non-null..
> 
> 
> 
>     Hmm, called object_resolve_partial_path() also doesn't set ambiguous on every path, so this hunk is at lease incomplete.
> 
> 
> yeah, but object_resolve_path_type() initializes it.
> 
>     I'm unsure about what semantics expected around ambigous pointers, but it seems to me that it is set only on failure paths, as a reason, why we failed. If this is true, I think, we need only the second hunk, which initializes local "ambig".
> 
> 
> right, and that seems good enough.
> 
> Do you ack/rb this change then?
> 
> 
>      qom/object: fix -Werror=maybe-uninitialized
> 
>      object_resolve_path_type() didn't always set *ambiguousp.
> 
>      Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com <mailto:marcandre.lureau@redhat.com>>
> 
> diff --git a/qom/object.c b/qom/object.c
> index 28c5b66eab..d3d3003541 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -2226,7 +2226,7 @@ Object *object_resolve_path_at(Object *parent, const char *path)
> 
>   Object *object_resolve_type_unambiguous(const char *typename, Error **errp)
>   {
> -    bool ambig;
> +    bool ambig = false;
>       Object *o = object_resolve_path_type("", typename, &ambig);
> 
>       if (ambig) {
> 
> 

Yes, I think this one in isolation is OK:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>


> thanks!
> 
>      >       }
>      >
>      >       g_strfreev(parts);
>      > @@ -2226,7 +2229,7 @@ Object *object_resolve_path_at(Object *parent, const char *path)
>      >
>      >   Object *object_resolve_type_unambiguous(const char *typename, Error **errp)
>      >   {
>      > -    bool ambig;
>      > +    bool ambig = false;
>      >       Object *o = object_resolve_path_type("", typename, &ambig);
>      >
>      >       if (ambig) {
> 
>     -- 
>     Best regards,
>     Vladimir
> 
> 
> 
> 
> -- 
> Marc-André Lureau

-- 
Best regards,
Vladimir

Re: [PATCH v3 21/22] qom/object: fix -Werror=maybe-uninitialized

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> object_resolve_path_type() didn't always set *ambiguousp.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Fixes: 81c48dd79655 (hw/i386/acpi: Add object_resolve_type_unambiguous to improve modularity)

> ---
>  qom/object.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index 28c5b66eab..bdc8a2c666 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -2201,6 +2201,9 @@ Object *object_resolve_path_type(const char *path, const char *typename,
>          }
>      } else {
>          obj = object_resolve_abs_path(object_get_root(), parts + 1, typename);
> +        if (ambiguousp) {
> +            *ambiguousp = false;
> +        }
>      }
>  
>      g_strfreev(parts);
> @@ -2226,7 +2229,7 @@ Object *object_resolve_path_at(Object *parent, const char *path)
>  
>  Object *object_resolve_type_unambiguous(const char *typename, Error **errp)
>  {
> -    bool ambig;
> +    bool ambig = false;
>      Object *o = object_resolve_path_type("", typename, &ambig);
>  
>      if (ambig) {

Contract:

   /**
    * object_resolve_path_type:
    * @path: the path to resolve
    * @typename: the type to look for.
    * @ambiguous: returns true if the path resolution failed because of an
    *   ambiguous match
    *
    * This is similar to object_resolve_path.  However, when looking for a
    * partial path only matches that implement the given type are considered.
    * This restricts the search and avoids spuriously flagging matches as
    * ambiguous.
    *
    * For both partial and absolute paths, the return value goes through
    * a dynamic cast to @typename.  This is important if either the link,
    * or the typename itself are of interface types.
    *
    * Returns: The matched object or NULL on path lookup failure.
    */

Note the parameter is called @ambiguous here, but @ambiguousp in the
definition.  Bad practice.

All the contract promises is that true will be stored in the variable
passed to @ambiguous when the function fails in a certain way.  For that
to work, the variable must be initialized to false.

You found a caller that doesn't: object_resolve_type_unambiguous().
This is a bug.  There might be more.  Impact is not obvious.

Two ways to fix:

1. Find all callers that don't, and fix them.  Your first hunk is then
   superfluous.  Your second hunk fixes the one you already found.

2. Change the contract so callers don't have to initialize.  Your second
   hunk is then superfluous.  The update to the contract is missing.

While there: the contract fails to specify that @ambiguous may be null.
Needs fixing, too.

Same for object_resolve_path().

Re: [PATCH v3 21/22] qom/object: fix -Werror=maybe-uninitialized

[Marc-André Lureau]

Hi

On Wed, Oct 2, 2024 at 10:21 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > object_resolve_path_type() didn't always set *ambiguousp.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Fixes: 81c48dd79655 (hw/i386/acpi: Add object_resolve_type_unambiguous to improve modularity)
>

ok

> > ---
> >  qom/object.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/qom/object.c b/qom/object.c
> > index 28c5b66eab..bdc8a2c666 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -2201,6 +2201,9 @@ Object *object_resolve_path_type(const char *path, const char *typename,
> >          }
> >      } else {
> >          obj = object_resolve_abs_path(object_get_root(), parts + 1, typename);
> > +        if (ambiguousp) {
> > +            *ambiguousp = false;
> > +        }
> >      }
> >
> >      g_strfreev(parts);
> > @@ -2226,7 +2229,7 @@ Object *object_resolve_path_at(Object *parent, const char *path)
> >
> >  Object *object_resolve_type_unambiguous(const char *typename, Error **errp)
> >  {
> > -    bool ambig;
> > +    bool ambig = false;
> >      Object *o = object_resolve_path_type("", typename, &ambig);
> >
> >      if (ambig) {
>
> Contract:
>
>    /**
>     * object_resolve_path_type:
>     * @path: the path to resolve
>     * @typename: the type to look for.
>     * @ambiguous: returns true if the path resolution failed because of an
>     *   ambiguous match
>     *
>     * This is similar to object_resolve_path.  However, when looking for a
>     * partial path only matches that implement the given type are considered.
>     * This restricts the search and avoids spuriously flagging matches as
>     * ambiguous.
>     *
>     * For both partial and absolute paths, the return value goes through
>     * a dynamic cast to @typename.  This is important if either the link,
>     * or the typename itself are of interface types.
>     *
>     * Returns: The matched object or NULL on path lookup failure.
>     */
>
> Note the parameter is called @ambiguous here, but @ambiguousp in the
> definition.  Bad practice.

hmm

>
> All the contract promises is that true will be stored in the variable
> passed to @ambiguous when the function fails in a certain way.  For that
> to work, the variable must be initialized to false.
>
> You found a caller that doesn't: object_resolve_type_unambiguous().
> This is a bug.  There might be more.  Impact is not obvious.
>
> Two ways to fix:
>
> 1. Find all callers that don't, and fix them.  Your first hunk is then
>    superfluous.  Your second hunk fixes the one you already found.
>

Imho, that's not a good API, it's easy to get wrong.

> 2. Change the contract so callers don't have to initialize.  Your second
>    hunk is then superfluous.  The update to the contract is missing.
>

I prefer that it always set the variable. I also prefer that caller
initializes variables. So all are good practices imho, even if it's a
bit redundant.

> While there: the contract fails to specify that @ambiguous may be null.
> Needs fixing, too.
>
> Same for object_resolve_path().
>

ok

Re: [PATCH v3 21/22] qom/object: fix -Werror=maybe-uninitialized

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
>
> On Wed, Oct 2, 2024 at 10:21 AM Markus Armbruster <armbru@redhat.com> wrote:

[...]

>> Two ways to fix:
>>
>> 1. Find all callers that don't, and fix them.  Your first hunk is then
>>    superfluous.  Your second hunk fixes the one you already found.
>>
>
> Imho, that's not a good API, it's easy to get wrong.
>
>> 2. Change the contract so callers don't have to initialize.  Your second
>>    hunk is then superfluous.  The update to the contract is missing.
>>
>
> I prefer that it always set the variable. I also prefer that caller
> initializes variables. So all are good practices imho, even if it's a
> bit redundant.

Since you're doing the work to fix the bug, you get first dibs on how to
fix it :)

[...]

[PATCH v3 22/22] fsdep/9p: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../fsdev/9p-iov-marshal.c:93:23: error: ‘val’ may be used uninitialized [-Werror=maybe-uninitialized]
and similar

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 fsdev/9p-iov-marshal.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fsdev/9p-iov-marshal.c b/fsdev/9p-iov-marshal.c
index a1c9beddd2..75fb87a490 100644
--- a/fsdev/9p-iov-marshal.c
+++ b/fsdev/9p-iov-marshal.c
@@ -84,7 +84,7 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int out_num, size_t offset,
             break;
         }
         case 'w': {
-            uint16_t val, *valp;
+            uint16_t val = 0, *valp;
             valp = va_arg(ap, uint16_t *);
             copied = v9fs_unpack(&val, out_sg, out_num, offset, sizeof(val));
             if (bswap) {
@@ -95,7 +95,7 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int out_num, size_t offset,
             break;
         }
         case 'd': {
-            uint32_t val, *valp;
+            uint32_t val = 0, *valp;
             valp = va_arg(ap, uint32_t *);
             copied = v9fs_unpack(&val, out_sg, out_num, offset, sizeof(val));
             if (bswap) {
@@ -106,7 +106,7 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int out_num, size_t offset,
             break;
         }
         case 'q': {
-            uint64_t val, *valp;
+            uint64_t val = 0, *valp;
             valp = va_arg(ap, uint64_t *);
             copied = v9fs_unpack(&val, out_sg, out_num, offset, sizeof(val));
             if (bswap) {
-- 
2.45.2.827.g557ae147e6

Re: [PATCH v3 22/22] fsdep/9p: fix -Werror=maybe-uninitialized false-positive

[Christian Schoenebeck via]

On Monday, September 30, 2024 10:14:57 AM CEST Marc-André Lureau wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> ../fsdev/9p-iov-marshal.c:93:23: error: ‘val’ may be used uninitialized [-Werror=maybe-uninitialized]
> and similar
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  fsdev/9p-iov-marshal.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fsdev/9p-iov-marshal.c b/fsdev/9p-iov-marshal.c
> index a1c9beddd2..75fb87a490 100644
> --- a/fsdev/9p-iov-marshal.c
> +++ b/fsdev/9p-iov-marshal.c
> @@ -84,7 +84,7 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int out_num, size_t offset,
>              break;
>          }
>          case 'w': {
> -            uint16_t val, *valp;
> +            uint16_t val = 0, *valp;
>              valp = va_arg(ap, uint16_t *);
>              copied = v9fs_unpack(&val, out_sg, out_num, offset, sizeof(val));

Another option would be inserting here:

	if (copied <= 0) break;

Same below. But I leave it up to you:

Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

/Christian

>              if (bswap) {
> @@ -95,7 +95,7 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int out_num, size_t offset,
>              break;
>          }
>          case 'd': {
> -            uint32_t val, *valp;
> +            uint32_t val = 0, *valp;
>              valp = va_arg(ap, uint32_t *);
>              copied = v9fs_unpack(&val, out_sg, out_num, offset, sizeof(val));
>              if (bswap) {
> @@ -106,7 +106,7 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int out_num, size_t offset,
>              break;
>          }
>          case 'q': {
> -            uint64_t val, *valp;
> +            uint64_t val = 0, *valp;
>              valp = va_arg(ap, uint64_t *);
>              copied = v9fs_unpack(&val, out_sg, out_num, offset, sizeof(val));
>              if (bswap) {
> 

Re: [PATCH v3 22/22] fsdep/9p: fix -Werror=maybe-uninitialized false-positive

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2288 bytes --]

Hi Christian

On Mon, Sep 30, 2024 at 1:27 PM Christian Schoenebeck via <
qemu-devel@nongnu.org> wrote:

> On Monday, September 30, 2024 10:14:57 AM CEST Marc-André Lureau wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > ../fsdev/9p-iov-marshal.c:93:23: error: ‘val’ may be used uninitialized
> [-Werror=maybe-uninitialized]
> > and similar
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  fsdev/9p-iov-marshal.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/fsdev/9p-iov-marshal.c b/fsdev/9p-iov-marshal.c
> > index a1c9beddd2..75fb87a490 100644
> > --- a/fsdev/9p-iov-marshal.c
> > +++ b/fsdev/9p-iov-marshal.c
> > @@ -84,7 +84,7 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int
> out_num, size_t offset,
> >              break;
> >          }
> >          case 'w': {
> > -            uint16_t val, *valp;
> > +            uint16_t val = 0, *valp;
> >              valp = va_arg(ap, uint16_t *);
> >              copied = v9fs_unpack(&val, out_sg, out_num, offset,
> sizeof(val));
>
> Another option would be inserting here:
>
>         if (copied <= 0) break;
>
> Same below. But I leave it up to you:
>
> Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
>
>
Ok, done

thanks


> /Christian
>
> >              if (bswap) {
> > @@ -95,7 +95,7 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int
> out_num, size_t offset,
> >              break;
> >          }
> >          case 'd': {
> > -            uint32_t val, *valp;
> > +            uint32_t val = 0, *valp;
> >              valp = va_arg(ap, uint32_t *);
> >              copied = v9fs_unpack(&val, out_sg, out_num, offset,
> sizeof(val));
> >              if (bswap) {
> > @@ -106,7 +106,7 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg,
> int out_num, size_t offset,
> >              break;
> >          }
> >          case 'q': {
> > -            uint64_t val, *valp;
> > +            uint64_t val = 0, *valp;
> >              valp = va_arg(ap, uint64_t *);
> >              copied = v9fs_unpack(&val, out_sg, out_num, offset,
> sizeof(val));
> >              if (bswap) {
> >
>
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3521 bytes --]

Re: [PATCH v3 00/22] -Werror=maybe-uninitialized fixes

[Marc-André Lureau]

Hi

On Mon, Sep 30, 2024 at 12:15 PM <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Hi,
>
> Depending on -Doptimization=<value>, GCC (14.2.1 here) produces different
> maybe-uninitialized warnings:
> - g: produces -Werror=maybe-uninitialized errors
> - 0: clean build
> - 1: produces -Werror=maybe-uninitialized errors
> - 2: clean build
> - 3: produces few -Werror=maybe-uninitialized errors
> - s: produces -Werror=maybe-uninitialized errors
>
> Most are false-positive, because prior LOCK_GUARD should guarantee an
> initialization path. Few of them are a bit trickier. Finally, I found
> a potential related memory leak.
>
> Patches missing r-b: 6, 10-11, 15-21

Patches missing review: 16, 20, 21.
"target/loongarch: fix -Werror=maybe-uninitialized false-positive"
"block: fix -Werror=maybe-uninitialized false-positive"
"qom/object: fix -Werror=maybe-uninitialized"

thanks!