From: Zenghui Yu <yuzenghui@huawei.com>

Quote docs/devel/style.rst (section "Automatic memory deallocation"):

* Variables declared with g_auto* MUST always be initialized,

otherwise the cleanup function will use uninitialized stack memory

Initialize @name properly to get rid of the compilation error (using

gcc-7.3.0 on CentOS):

../hw/remote/proxy.c: In function 'pci_proxy_dev_realize':

/usr/include/glib-2.0/glib/glib-autocleanups.h:28:3: error: 'name' may be used uninitialized in this function [-Werror=maybe-uninitialized]

g_free (*pp);

^~~~~~~~~~~~

../hw/remote/proxy.c:350:30: note: 'name' was declared here

g_autofree char *name;

^~~~

Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>

Reviewed-by: Jagannathan Raman <jag.raman@oracle.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Miroslav Rezanina <mrezanin@redhat.com>

Message-id: 20210312112143.1369-1-yuzenghui@huawei.com

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

---

hw/remote/memory.c | 5 ++---

hw/remote/proxy.c | 3 +--

2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/remote/memory.c b/hw/remote/memory.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/remote/memory.c

+++ b/hw/remote/memory.c

@@ -XXX,XX +XXX,XX @@ void remote_sysmem_reconfig(MPQemuMsg *msg, Error **errp)

remote_sysmem_reset();

- for (region = 0; region < msg->num_fds; region++) {

- g_autofree char *name;

+ for (region = 0; region < msg->num_fds; region++, suffix++) {

+ g_autofree char *name = g_strdup_printf("remote-mem-%u", suffix);

subregion = g_new(MemoryRegion, 1);

- name = g_strdup_printf("remote-mem-%u", suffix++);

memory_region_init_ram_from_fd(subregion, NULL,

name, sysmem_info->sizes[region],

true, msg->fds[region],

diff --git a/hw/remote/proxy.c b/hw/remote/proxy.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/remote/proxy.c

+++ b/hw/remote/proxy.c

@@ -XXX,XX +XXX,XX @@ static void probe_pci_info(PCIDevice *dev, Error **errp)

PCI_BASE_ADDRESS_SPACE_IO : PCI_BASE_ADDRESS_SPACE_MEMORY;

if (size) {

- g_autofree char *name;

+ g_autofree char *name = g_strdup_printf("bar-region-%d", i);

pdev->region[i].dev = pdev;

pdev->region[i].present = true;

if (type == PCI_BASE_ADDRESS_SPACE_MEMORY) {

pdev->region[i].memory = true;

}

- name = g_strdup_printf("bar-region-%d", i);

memory_region_init_io(&pdev->region[i].mr, OBJECT(pdev),

&proxy_mr_ops, &pdev->region[i],

name, size);

--

2.31.1

From: Paolo Bonzini <pbonzini@redhat.com>

The lifetime of the timer is well-known (it cannot outlive

qemu_co_sleep_ns_wakeable, because it's deleted by the time the

coroutine resumes), so it is not necessary to place it on the heap.

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Message-id: 20210517100548.28806-2-pbonzini@redhat.com

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

---

util/qemu-coroutine-sleep.c | 9 ++++-----

1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c

index XXXXXXX..XXXXXXX 100644

--- a/util/qemu-coroutine-sleep.c

+++ b/util/qemu-coroutine-sleep.c

@@ -XXX,XX +XXX,XX @@ static const char *qemu_co_sleep_ns__scheduled = "qemu_co_sleep_ns";

struct QemuCoSleepState {

Coroutine *co;

- QEMUTimer *ts;

+ QEMUTimer ts;

QemuCoSleepState **user_state_pointer;

};

@@ -XXX,XX +XXX,XX @@ void qemu_co_sleep_wake(QemuCoSleepState *sleep_state)

if (sleep_state->user_state_pointer) {

*sleep_state->user_state_pointer = NULL;

}

- timer_del(sleep_state->ts);

+ timer_del(&sleep_state->ts);

aio_co_wake(sleep_state->co);

}

@@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,

AioContext *ctx = qemu_get_current_aio_context();

QemuCoSleepState state = {

.co = qemu_coroutine_self(),

- .ts = aio_timer_new(ctx, type, SCALE_NS, co_sleep_cb, &state),

.user_state_pointer = sleep_state,

};

@@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,

abort();

}

+ aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, &state);

if (sleep_state) {

*sleep_state = &state;

}

- timer_mod(state.ts, qemu_clock_get_ns(type) + ns);

+ timer_mod(&state.ts, qemu_clock_get_ns(type) + ns);

qemu_coroutine_yield();

if (sleep_state) {

/*

@@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,

*/

assert(*sleep_state == NULL);

}

- timer_free(state.ts);

}

--

2.31.1

From: Paolo Bonzini <pbonzini@redhat.com>

Simplify the code by removing conditionals. qemu_co_sleep_ns

can simply point the argument to an on-stack temporary.

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Message-id: 20210517100548.28806-3-pbonzini@redhat.com

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

---

include/qemu/coroutine.h | 5 +++--

util/qemu-coroutine-sleep.c | 18 +++++-------------

2 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h

index XXXXXXX..XXXXXXX 100644

--- a/include/qemu/coroutine.h

+++ b/include/qemu/coroutine.h

@@ -XXX,XX +XXX,XX @@ typedef struct QemuCoSleepState QemuCoSleepState;

/**

* Yield the coroutine for a given duration. During this yield, @sleep_state

- * (if not NULL) is set to an opaque pointer, which may be used for

+ * is set to an opaque pointer, which may be used for

* qemu_co_sleep_wake(). Be careful, the pointer is set back to zero when the

* timer fires. Don't save the obtained value to other variables and don't call

* qemu_co_sleep_wake from another aio context.

@@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,

QemuCoSleepState **sleep_state);

static inline void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)

{

- qemu_co_sleep_ns_wakeable(type, ns, NULL);

+ QemuCoSleepState *unused = NULL;

+ qemu_co_sleep_ns_wakeable(type, ns, &unused);

}

/**

diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c

index XXXXXXX..XXXXXXX 100644

--- a/util/qemu-coroutine-sleep.c

+++ b/util/qemu-coroutine-sleep.c

@@ -XXX,XX +XXX,XX @@ void qemu_co_sleep_wake(QemuCoSleepState *sleep_state)

qemu_co_sleep_ns__scheduled, NULL);

assert(scheduled == qemu_co_sleep_ns__scheduled);

- if (sleep_state->user_state_pointer) {

- *sleep_state->user_state_pointer = NULL;

- }

+ *sleep_state->user_state_pointer = NULL;

timer_del(&sleep_state->ts);

aio_co_wake(sleep_state->co);

}

@@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,

}

aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, &state);

- if (sleep_state) {

- *sleep_state = &state;

- }

+ *sleep_state = &state;

timer_mod(&state.ts, qemu_clock_get_ns(type) + ns);

qemu_coroutine_yield();

- if (sleep_state) {

- /*

- * Note that *sleep_state is cleared during qemu_co_sleep_wake

- * before resuming this coroutine.

- */

- assert(*sleep_state == NULL);

- }

+

+ /* qemu_co_sleep_wake clears *sleep_state before resuming this coroutine. */

+ assert(*sleep_state == NULL);

}

--

2.31.1

From: Paolo Bonzini <pbonzini@redhat.com>

All callers of qemu_co_sleep_wake are checking whether they are passing

a NULL argument inside the pointer-to-pointer: do the check in

qemu_co_sleep_wake itself.

As a side effect, qemu_co_sleep_wake can be called more than once and

it will only wake the coroutine once; after the first time, the argument

will be set to NULL via *sleep_state->user_state_pointer. However, this

would not be safe unless co_sleep_cb keeps using the QemuCoSleepState*

directly, so make it go through the pointer-to-pointer instead.

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Message-id: 20210517100548.28806-4-pbonzini@redhat.com

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

---

block/block-copy.c | 4 +---

block/nbd.c | 8 ++------

util/qemu-coroutine-sleep.c | 21 ++++++++++++---------

3 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c

index XXXXXXX..XXXXXXX 100644

--- a/block/block-copy.c

+++ b/block/block-copy.c

@@ -XXX,XX +XXX,XX @@ out:

void block_copy_kick(BlockCopyCallState *call_state)

{

- if (call_state->sleep_state) {

- qemu_co_sleep_wake(call_state->sleep_state);

- }

+ qemu_co_sleep_wake(call_state->sleep_state);

}

/*

diff --git a/block/nbd.c b/block/nbd.c

index XXXXXXX..XXXXXXX 100644

--- a/block/nbd.c

+++ b/block/nbd.c

@@ -XXX,XX +XXX,XX @@ static void coroutine_fn nbd_client_co_drain_begin(BlockDriverState *bs)

BDRVNBDState *s = (BDRVNBDState *)bs->opaque;

s->drained = true;

- if (s->connection_co_sleep_ns_state) {

- qemu_co_sleep_wake(s->connection_co_sleep_ns_state);

- }

+ qemu_co_sleep_wake(s->connection_co_sleep_ns_state);

nbd_co_establish_connection_cancel(bs, false);

@@ -XXX,XX +XXX,XX @@ static void nbd_teardown_connection(BlockDriverState *bs)

s->state = NBD_CLIENT_QUIT;

if (s->connection_co) {

- if (s->connection_co_sleep_ns_state) {

- qemu_co_sleep_wake(s->connection_co_sleep_ns_state);

- }

+ qemu_co_sleep_wake(s->connection_co_sleep_ns_state);

nbd_co_establish_connection_cancel(bs, true);

}

if (qemu_in_coroutine()) {

diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c

index XXXXXXX..XXXXXXX 100644

--- a/util/qemu-coroutine-sleep.c

+++ b/util/qemu-coroutine-sleep.c

@@ -XXX,XX +XXX,XX @@ struct QemuCoSleepState {

void qemu_co_sleep_wake(QemuCoSleepState *sleep_state)

{

- /* Write of schedule protected by barrier write in aio_co_schedule */

- const char *scheduled = qatomic_cmpxchg(&sleep_state->co->scheduled,

- qemu_co_sleep_ns__scheduled, NULL);

+ if (sleep_state) {

+ /* Write of schedule protected by barrier write in aio_co_schedule */

+ const char *scheduled = qatomic_cmpxchg(&sleep_state->co->scheduled,

+ qemu_co_sleep_ns__scheduled, NULL);

- assert(scheduled == qemu_co_sleep_ns__scheduled);

- *sleep_state->user_state_pointer = NULL;

- timer_del(&sleep_state->ts);

- aio_co_wake(sleep_state->co);

+ assert(scheduled == qemu_co_sleep_ns__scheduled);

+ *sleep_state->user_state_pointer = NULL;

+ timer_del(&sleep_state->ts);

+ aio_co_wake(sleep_state->co);

+ }

}

static void co_sleep_cb(void *opaque)

{

- qemu_co_sleep_wake(opaque);

+ QemuCoSleepState **sleep_state = opaque;

+ qemu_co_sleep_wake(*sleep_state);

}

void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,

@@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,

abort();

}

- aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, &state);

+ aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, sleep_state);

*sleep_state = &state;

timer_mod(&state.ts, qemu_clock_get_ns(type) + ns);

qemu_coroutine_yield();

--

2.31.1

From: Paolo Bonzini <pbonzini@redhat.com>

This simplification is enabled by the previous patch. Now aio_co_wake

will only be called once, therefore we do not care about a spurious

firing of the timer after a qemu_co_sleep_wake.

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Message-id: 20210517100548.28806-5-pbonzini@redhat.com

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

---

util/qemu-coroutine-sleep.c | 8 ++++----

1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c

index XXXXXXX..XXXXXXX 100644

--- a/util/qemu-coroutine-sleep.c

+++ b/util/qemu-coroutine-sleep.c

@@ -XXX,XX +XXX,XX @@ static const char *qemu_co_sleep_ns__scheduled = "qemu_co_sleep_ns";

struct QemuCoSleepState {

Coroutine *co;

- QEMUTimer ts;

QemuCoSleepState **user_state_pointer;

};

@@ -XXX,XX +XXX,XX @@ void qemu_co_sleep_wake(QemuCoSleepState *sleep_state)

assert(scheduled == qemu_co_sleep_ns__scheduled);

*sleep_state->user_state_pointer = NULL;

- timer_del(&sleep_state->ts);

aio_co_wake(sleep_state->co);

}

}

@@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,

QemuCoSleepState **sleep_state)

{

AioContext *ctx = qemu_get_current_aio_context();

+ QEMUTimer ts;

QemuCoSleepState state = {

.co = qemu_coroutine_self(),

.user_state_pointer = sleep_state,

@@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,

abort();

}

- aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, sleep_state);

+ aio_timer_init(ctx, &ts, type, SCALE_NS, co_sleep_cb, sleep_state);

*sleep_state = &state;

- timer_mod(&state.ts, qemu_clock_get_ns(type) + ns);

+ timer_mod(&ts, qemu_clock_get_ns(type) + ns);

qemu_coroutine_yield();

+ timer_del(&ts);

/* qemu_co_sleep_wake clears *sleep_state before resuming this coroutine. */

assert(*sleep_state == NULL);

--

2.31.1