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

[PATCH v2 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>
---
 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..d700986c39 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 = -1;
+
         sum = 0;
         for (i = 0; i < prdtl; i++) {
             tbl_entry_size = prdt_tbl_entry_size(&tbl[i]);
-- 
2.45.2.827.g557ae147e6

[PATCH v2 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.

thanks

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 false-positive
  block: fix -Werror=maybe-uninitialized false-positive
  qom/object: fix -Werror=maybe-uninitialized
  fsdep/9p: fix -Werror=maybe-uninitialized false-positive
  RFC: hw/virtio: a potential leak fix

 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 +++---
 22 files changed, 57 insertions(+), 49 deletions(-)

-- 
2.45.2.827.g557ae147e6

[PATCH v2 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 v2 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>
---
 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 213114be68..c948cb1b72 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 v2 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>
---
 hw/display/qxl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 3c2b5182ca..20e13ee98d 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 = -1;
     pcibus_t pci_end;
     MemoryRegion *mr;
     intptr_t virt_start;
-- 
2.45.2.827.g557ae147e6

[PATCH v2 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 v2 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 v2 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]

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..fd55a580fb 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;
+    uint64_t 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

[PATCH v2 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>
---
 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 v2 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>
---
 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..d700986c39 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 = -1;
+
         sum = 0;
         for (i = 0; i < prdtl; i++) {
             tbl_entry_size = prdt_tbl_entry_size(&tbl[i]);
-- 
2.45.2.827.g557ae147e6

[PATCH v2 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>
---
 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 v2 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

[PATCH v2 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

[PATCH v2 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 1d9db81299..32f398a754 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 v2 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>
---
 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 v2 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 67ca3d5d51..502368a3a6 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1795,7 +1795,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 v2 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

[PATCH v2 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

[PATCH v2 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

[PATCH v2 18/22] hw/virtio: fix -Werror=maybe-uninitialized false-positive

[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]

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..cd29cc795b 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -526,7 +526,7 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
 size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
 {
     size_t len = 0;
-    uint32_t r;
+    uint32_t r = 0;
 
     while (num--) {
         int64_t start_us = g_get_monotonic_time();
-- 
2.45.2.827.g557ae147e6

[PATCH v2 19/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

[PATCH v2 20/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 157a45c5f8..b2120f7dc5 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -2202,6 +2202,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);
@@ -2227,7 +2230,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

[PATCH v2 21/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

[PATCH v2 22/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 cd29cc795b..93742d9ddc 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)
 {
@@ -529,6 +530,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
     uint32_t r = 0;
 
     while (num--) {
+        g_autofree VirtQueueElement *elem = NULL;
         int64_t start_us = g_get_monotonic_time();
 
         do {
@@ -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 v2 00/22] -Werror=maybe-uninitialized fixes

[Daniel P. Berrangé]

On Tue, Sep 24, 2024 at 05:05:31PM +0400, 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.

In addition we now build with "-ftrivial-auto-var-init=zero", so
any case which is missing an "= NULL" or "= 0"  initialization is
protected.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Stefano Garzarella]

On Tue, Sep 24, 2024 at 05:05:40PM GMT, marcandre.lureau@redhat.com wrote:
>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.

I agree, for vhost_scsi_set_workers() point of view, it doesn't need to
add a new worker in that case, so it should be fine to return 0.

If we really want to fail when num_queues=0, maybe it should be in
vhost_scsi_realize().

>
>Signed-off-by: Marc-André Lureau <marcandre.lureau@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) {

Another option could have been to edit this check:
       if (!per_virtqueue || dev->nvqs <= VHOST_SCSI_VQ_NUM_FIXED + 1) {
           return 0;
       }

But I'm fine with your change:

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

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

[Stefano Garzarella]

On Tue, Sep 24, 2024 at 05:05:44PM GMT, marcandre.lureau@redhat.com wrote:
>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>
>---
> hw/block/virtio-blk.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

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

>
>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
>

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

[Stefano Garzarella]

On Tue, Sep 24, 2024 at 05:05:49PM GMT, marcandre.lureau@redhat.com wrote:
>From: Marc-André Lureau <marcandre.lureau@redhat.com>

For the title: I don't think it is a false positive, but a real fix,
indeed maybe not a complete one.

>
>../hw/virtio/vhost-shadow-virtqueue.c:545:13: error: ‘r’ may be used uninitialized [-Werror=maybe-uninitialized]
>
>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..cd29cc795b 100644
>--- a/hw/virtio/vhost-shadow-virtqueue.c
>+++ b/hw/virtio/vhost-shadow-virtqueue.c
>@@ -526,7 +526,7 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
> size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
> {
>     size_t len = 0;
>-    uint32_t r;
>+    uint32_t r = 0;
>
>     while (num--) {

I think we should move the initialization to 0 here in the loop:

           uint32_t r = 0;

>         int64_t start_us = g_get_monotonic_time();

...

           vhost_svq_get_buf(svq, &r);
           len += r;
       }

This because we don't check vhost_svq_get_buf() return value.

IIUC, in that function, `r` is set only if the return value of
vhost_svq_get_buf() is not null, so if we don't check its return value,
we should set `r` to 0 on every cycle (or check the return value of
course).

Thanks,
Stefano

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

[Stefano Garzarella]

On Tue, Sep 24, 2024 at 05:05:53PM GMT, 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.
>
>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 cd29cc795b..93742d9ddc 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)
> {
>@@ -529,6 +530,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
>     uint32_t r = 0;
>
>     while (num--) {
>+        g_autofree VirtQueueElement *elem = NULL;

Yes, indeed it sounds like we should release the buffer, although from
the name of the function here, it sounds like we are just trying to
figure out if the queue has elements, so I expect there is another
function that is then called to process the buffers.

There's still a potential problem here that I pointed out in the other
patch, but I think we need Eugenio here.

>         int64_t start_us = g_get_monotonic_time();
>
>         do {
>@@ -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 v2 02/22] util/timer: fix -Werror=maybe-uninitialized false-positive

[Manos Pitsidianakis]

On Tue, 24 Sep 2024 16:05, marcandre.lureau@redhat.com wrote:
>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>
>---
> 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 213114be68..c948cb1b72 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
>
>

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

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

[Manos Pitsidianakis]

On Tue, 24 Sep 2024 16:05, marcandre.lureau@redhat.com wrote:
>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>
>---
> hw/display/qxl.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/hw/display/qxl.c b/hw/display/qxl.c
>index 3c2b5182ca..20e13ee98d 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 = -1;

Suggestion:

-    pcibus_t pci_start;
+    pcibus_t pci_start = PCI_BAR_UNMAPPED;

Either way,

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

>     pcibus_t pci_end;
>     MemoryRegion *mr;
>     intptr_t virt_start;
>-- 
>2.45.2.827.g557ae147e6
>
>

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

[Manos Pitsidianakis]

On Tue, 24 Sep 2024 16:05, 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]
>
>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..fd55a580fb 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;
>+    uint64_t ret = -1;

This doesn't look right, `ret` should be int. It receives the return 
result of bdrv_co_preadv() which is int and is passed as an int argument 
to mirror_read_complete().

Not related to this patch obviously. But since you're touching this code 
maybe we could include this small cleanup?


>     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 v2 07/22] block/stream: fix -Werror=maybe-uninitialized false-positives

[Manos Pitsidianakis]

On Tue, 24 Sep 2024 16:05, marcandre.lureau@redhat.com wrote:
>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>
>---
> 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
>
>

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

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

[Manos Pitsidianakis]

On Tue, 24 Sep 2024 16:05, marcandre.lureau@redhat.com wrote:
>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>
>---
> 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..d700986c39 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 = -1;
>+

Semantically it should be 0 instead of -1.

Either way:

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

>         sum = 0;
>         for (i = 0; i < prdtl; i++) {
>             tbl_entry_size = prdt_tbl_entry_size(&tbl[i]);
>-- 
>2.45.2.827.g557ae147e6
>
>

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

[Eugenio Perez Martin]

On Tue, Sep 24, 2024 at 3:07 PM <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]
>
> 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..cd29cc795b 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -526,7 +526,7 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
>  size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
>  {
>      size_t len = 0;
> -    uint32_t r;
> +    uint32_t r = 0;
>

I understand this is a bulk changeset to avoid the warning, but does
this mean we cannot use pointer arguments to just return information
anymore? vhost_svq_get_buf just write to it, it never reads it.

If you post a second version and it is convenient for you, it would be
useful to move it inside of the while.

Any way we solve it,

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

>      while (num--) {
>          int64_t start_us = g_get_monotonic_time();
> --
> 2.45.2.827.g557ae147e6
>

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

[Eugenio Perez Martin]

On Wed, Sep 25, 2024 at 10:08 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Tue, Sep 24, 2024 at 05:05:49PM GMT, marcandre.lureau@redhat.com wrote:
> >From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> For the title: I don't think it is a false positive, but a real fix,
> indeed maybe not a complete one.
>
> >
> >../hw/virtio/vhost-shadow-virtqueue.c:545:13: error: ‘r’ may be used uninitialized [-Werror=maybe-uninitialized]
> >
> >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..cd29cc795b 100644
> >--- a/hw/virtio/vhost-shadow-virtqueue.c
> >+++ b/hw/virtio/vhost-shadow-virtqueue.c
> >@@ -526,7 +526,7 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
> > size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
> > {
> >     size_t len = 0;
> >-    uint32_t r;
> >+    uint32_t r = 0;
> >
> >     while (num--) {
>
> I think we should move the initialization to 0 here in the loop:
>
>            uint32_t r = 0;
>
> >         int64_t start_us = g_get_monotonic_time();
>
> ...
>
>            vhost_svq_get_buf(svq, &r);
>            len += r;
>        }
>
> This because we don't check vhost_svq_get_buf() return value.
>
> IIUC, in that function, `r` is set only if the return value of
> vhost_svq_get_buf() is not null, so if we don't check its return value,
> we should set `r` to 0 on every cycle (or check the return value of
> course).
>

Sorry I missed this mail and I proposed the same :). I do think it is
a real false positive though, in the sense that if we embed the
vhost_svq_get_buf here the warning would go away.

But I understand it is better to change this function than trust the
reviews long term.

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

[Stefano Garzarella]

On Fri, Sep 27, 2024 at 3:05 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Tue, Sep 24, 2024 at 3:07 PM <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]
> >
> > 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..cd29cc795b 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > @@ -526,7 +526,7 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
> >  size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
> >  {
> >      size_t len = 0;
> > -    uint32_t r;
> > +    uint32_t r = 0;
> >
>
> I understand this is a bulk changeset to avoid the warning, but does
> this mean we cannot use pointer arguments to just return information
> anymore? vhost_svq_get_buf just write to it, it never reads it.

Sure we can, the problem here is that vhost_svq_get_buf() might return
without having written there (in the error path).

>
> If you post a second version and it is convenient for you, it would be
> useful to move it inside of the while.

I think it is the only way, if we keep it out we have the problem from
the second loop on (always in the error path).

>
> Any way we solve it,
>
> Acked-by: Eugenio Pérez <eperezma@redhat.com>
>
> >      while (num--) {
> >          int64_t start_us = g_get_monotonic_time();
> > --
> > 2.45.2.827.g557ae147e6
> >
>

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

[Stefano Garzarella]

On Fri, Sep 27, 2024 at 3:08 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Wed, Sep 25, 2024 at 10:08 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >
> > On Tue, Sep 24, 2024 at 05:05:49PM GMT, marcandre.lureau@redhat.com wrote:
> > >From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > For the title: I don't think it is a false positive, but a real fix,
> > indeed maybe not a complete one.
> >
> > >
> > >../hw/virtio/vhost-shadow-virtqueue.c:545:13: error: ‘r’ may be used uninitialized [-Werror=maybe-uninitialized]
> > >
> > >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..cd29cc795b 100644
> > >--- a/hw/virtio/vhost-shadow-virtqueue.c
> > >+++ b/hw/virtio/vhost-shadow-virtqueue.c
> > >@@ -526,7 +526,7 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
> > > size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
> > > {
> > >     size_t len = 0;
> > >-    uint32_t r;
> > >+    uint32_t r = 0;
> > >
> > >     while (num--) {
> >
> > I think we should move the initialization to 0 here in the loop:
> >
> >            uint32_t r = 0;
> >
> > >         int64_t start_us = g_get_monotonic_time();
> >
> > ...
> >
> >            vhost_svq_get_buf(svq, &r);
> >            len += r;
> >        }
> >
> > This because we don't check vhost_svq_get_buf() return value.
> >
> > IIUC, in that function, `r` is set only if the return value of
> > vhost_svq_get_buf() is not null, so if we don't check its return value,
> > we should set `r` to 0 on every cycle (or check the return value of
> > course).
> >
>
> Sorry I missed this mail and I proposed the same :). I do think it is
> a real false positive though, in the sense that if we embed the
> vhost_svq_get_buf here the warning would go away.

I don't think so, I mean if we embed it and check the error path
better, yes, but now in vhost_svq_get_buf() if we fail, we return
NULL, but we don't set "len" to 0, so we would have the same warning.

Thanks,
Stefano

>
> But I understand it is better to change this function than trust the
> reviews long term.
>

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

[Eugenio Perez Martin]

On Mon, Sep 30, 2024 at 10:11 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Fri, Sep 27, 2024 at 3:08 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Wed, Sep 25, 2024 at 10:08 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > >
> > > On Tue, Sep 24, 2024 at 05:05:49PM GMT, marcandre.lureau@redhat.com wrote:
> > > >From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > For the title: I don't think it is a false positive, but a real fix,
> > > indeed maybe not a complete one.
> > >
> > > >
> > > >../hw/virtio/vhost-shadow-virtqueue.c:545:13: error: ‘r’ may be used uninitialized [-Werror=maybe-uninitialized]
> > > >
> > > >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..cd29cc795b 100644
> > > >--- a/hw/virtio/vhost-shadow-virtqueue.c
> > > >+++ b/hw/virtio/vhost-shadow-virtqueue.c
> > > >@@ -526,7 +526,7 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
> > > > size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
> > > > {
> > > >     size_t len = 0;
> > > >-    uint32_t r;
> > > >+    uint32_t r = 0;
> > > >
> > > >     while (num--) {
> > >
> > > I think we should move the initialization to 0 here in the loop:
> > >
> > >            uint32_t r = 0;
> > >
> > > >         int64_t start_us = g_get_monotonic_time();
> > >
> > > ...
> > >
> > >            vhost_svq_get_buf(svq, &r);
> > >            len += r;
> > >        }
> > >
> > > This because we don't check vhost_svq_get_buf() return value.
> > >
> > > IIUC, in that function, `r` is set only if the return value of
> > > vhost_svq_get_buf() is not null, so if we don't check its return value,
> > > we should set `r` to 0 on every cycle (or check the return value of
> > > course).
> > >
> >
> > Sorry I missed this mail and I proposed the same :). I do think it is
> > a real false positive though, in the sense that if we embed the
> > vhost_svq_get_buf here the warning would go away.
>
> I don't think so, I mean if we embed it and check the error path
> better, yes, but now in vhost_svq_get_buf() if we fail, we return
> NULL, but we don't set "len" to 0, so we would have the same warning.
>

Ohh got it, I missed that path!

> Thanks,
> Stefano
>
> >
> > But I understand it is better to change this function than trust the
> > reviews long term.
> >
>