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: Philippe Mathieu-Daudé <philmd@redhat.com>

Document the following functions return the bitmap size

if no matching bit is found:

- find_first_bit

- find_next_bit

- find_last_bit

- find_first_zero_bit

- find_next_zero_bit

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

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

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

Message-id: 20210510200758.2623154-2-philmd@redhat.com

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

---

include/qemu/bitops.h | 15 ++++++++++++---

1 file changed, 12 insertions(+), 3 deletions(-)

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

index XXXXXXX..XXXXXXX 100644

--- a/include/qemu/bitops.h

+++ b/include/qemu/bitops.h

@@ -XXX,XX +XXX,XX @@ static inline int test_bit(long nr, const unsigned long *addr)

* @addr: The address to start the search at

* @size: The maximum size to search

*

- * Returns the bit number of the first set bit, or size.

+ * Returns the bit number of the last set bit,

+ * or @size if there is no set bit in the bitmap.

*/

unsigned long find_last_bit(const unsigned long *addr,

unsigned long size);

@@ -XXX,XX +XXX,XX @@ unsigned long find_last_bit(const unsigned long *addr,

* @addr: The address to base the search on

* @offset: The bitnumber to start searching at

* @size: The bitmap size in bits

+ *

+ * Returns the bit number of the next set bit,

+ * or @size if there are no further set bits in the bitmap.

*/

unsigned long find_next_bit(const unsigned long *addr,

unsigned long size,

@@ -XXX,XX +XXX,XX @@ unsigned long find_next_bit(const unsigned long *addr,

* @addr: The address to base the search on

* @offset: The bitnumber to start searching at

* @size: The bitmap size in bits

+ *

+ * Returns the bit number of the next cleared bit,

+ * or @size if there are no further clear bits in the bitmap.

*/

unsigned long find_next_zero_bit(const unsigned long *addr,

@@ -XXX,XX +XXX,XX @@ unsigned long find_next_zero_bit(const unsigned long *addr,

* @addr: The address to start the search at

* @size: The maximum size to search

*

- * Returns the bit number of the first set bit.

+ * Returns the bit number of the first set bit,

+ * or @size if there is no set bit in the bitmap.

*/

static inline unsigned long find_first_bit(const unsigned long *addr,

unsigned long size)

@@ -XXX,XX +XXX,XX @@ static inline unsigned long find_first_bit(const unsigned long *addr,

* @addr: The address to start the search at

* @size: The maximum size to search

*

- * Returns the bit number of the first cleared bit.

+ * Returns the bit number of the first cleared bit,

+ * or @size if there is no clear bit in the bitmap.

*/

static inline unsigned long find_first_zero_bit(const unsigned long *addr,

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

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>

Right now, users of qemu_co_sleep_ns_wakeable are simply passing

a pointer to QemuCoSleepState by reference to the function. But

QemuCoSleepState really is just a Coroutine*; making the

content of the struct public is just as efficient and lets us

skip the user_state_pointer indirection.

Since the usage is changed, take the occasion to rename the

struct to QemuCoSleep.

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

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

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

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

---

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

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

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

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

4 files changed, 39 insertions(+), 43 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 @@ void qemu_co_rwlock_wrlock(CoRwlock *lock);

*/

void qemu_co_rwlock_unlock(CoRwlock *lock);

-typedef struct QemuCoSleepState QemuCoSleepState;

+typedef struct QemuCoSleep {

+ Coroutine *to_wake;

+} QemuCoSleep;

/**

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

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

+ * Yield the coroutine for a given duration. Initializes @w so that,

+ * during this yield, it can be passed to qemu_co_sleep_wake() to

+ * terminate the sleep.

*/

-void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,

- QemuCoSleepState **sleep_state);

+void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w,

+ QEMUClockType type, int64_t ns);

+

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

{

- QemuCoSleepState *unused = NULL;

- qemu_co_sleep_ns_wakeable(type, ns, &unused);

+ QemuCoSleep w = { 0 };

+ qemu_co_sleep_ns_wakeable(&w, type, ns);

}

/**

@@ -XXX,XX +XXX,XX @@ static inline void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)

* qemu_co_sleep_ns() and should be checked to be non-NULL before calling

* qemu_co_sleep_wake().

*/

-void qemu_co_sleep_wake(QemuCoSleepState *sleep_state);

+void qemu_co_sleep_wake(QemuCoSleep *w);

/**

* Yield until a file descriptor becomes readable

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 @@ typedef struct BlockCopyCallState {

/* State */

int ret;

bool finished;

- QemuCoSleepState *sleep_state;

+ QemuCoSleep sleep;

bool cancelled;

/* OUT parameters */

@@ -XXX,XX +XXX,XX @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)

if (ns > 0) {

block_copy_task_end(task, -EAGAIN);

g_free(task);

- qemu_co_sleep_ns_wakeable(QEMU_CLOCK_REALTIME, ns,

- &call_state->sleep_state);

+ qemu_co_sleep_ns_wakeable(&call_state->sleep,

+ QEMU_CLOCK_REALTIME, ns);

continue;

}

}

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

void block_copy_kick(BlockCopyCallState *call_state)

{

- qemu_co_sleep_wake(call_state->sleep_state);

+ qemu_co_sleep_wake(&call_state->sleep);

}

/*

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 @@ typedef struct BDRVNBDState {

CoQueue free_sema;

Coroutine *connection_co;

Coroutine *teardown_co;

- QemuCoSleepState *connection_co_sleep_ns_state;

+ QemuCoSleep reconnect_sleep;

bool drained;

bool wait_drained_end;

int in_flight;

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

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

s->drained = true;

- qemu_co_sleep_wake(s->connection_co_sleep_ns_state);

+ qemu_co_sleep_wake(&s->reconnect_sleep);

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

- qemu_co_sleep_wake(s->connection_co_sleep_ns_state);

+ qemu_co_sleep_wake(&s->reconnect_sleep);

nbd_co_establish_connection_cancel(bs, true);

}

if (qemu_in_coroutine()) {

@@ -XXX,XX +XXX,XX @@ static coroutine_fn void nbd_co_reconnect_loop(BDRVNBDState *s)

}

bdrv_inc_in_flight(s->bs);

} else {

- qemu_co_sleep_ns_wakeable(QEMU_CLOCK_REALTIME, timeout,

- &s->connection_co_sleep_ns_state);

+ qemu_co_sleep_ns_wakeable(&s->reconnect_sleep,

+ QEMU_CLOCK_REALTIME, timeout);

if (s->drained) {

continue;

}

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 {

+void qemu_co_sleep_wake(QemuCoSleep *w)

+{

Coroutine *co;

- QemuCoSleepState **user_state_pointer;

-};

-void qemu_co_sleep_wake(QemuCoSleepState *sleep_state)

-{

- if (sleep_state) {

+ co = w->to_wake;

+ w->to_wake = NULL;

+ if (co) {

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

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

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

qemu_co_sleep_ns__scheduled, NULL);

assert(scheduled == qemu_co_sleep_ns__scheduled);

- *sleep_state->user_state_pointer = NULL;

- aio_co_wake(sleep_state->co);

+ aio_co_wake(co);

}

}

static void co_sleep_cb(void *opaque)

{

- QemuCoSleepState **sleep_state = opaque;

- qemu_co_sleep_wake(*sleep_state);

+ QemuCoSleep *w = opaque;

+ qemu_co_sleep_wake(w);

}

-void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,

- QemuCoSleepState **sleep_state)

+void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w,

+ QEMUClockType type, int64_t ns)

{

+ Coroutine *co = qemu_coroutine_self();

AioContext *ctx = qemu_get_current_aio_context();

QEMUTimer ts;

- QemuCoSleepState state = {

- .co = qemu_coroutine_self(),

- .user_state_pointer = sleep_state,

- };

- const char *scheduled = qatomic_cmpxchg(&state.co->scheduled, NULL,

- qemu_co_sleep_ns__scheduled);

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

+ qemu_co_sleep_ns__scheduled);

if (scheduled) {

fprintf(stderr,

"%s: Co-routine was already scheduled in '%s'\n",

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

abort();

}

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

- *sleep_state = &state;

+ w->to_wake = co;

+ aio_timer_init(ctx, &ts, type, SCALE_NS, co_sleep_cb, w),

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

+ /* w->to_wake is cleared before resuming this coroutine. */

+ assert(w->to_wake == NULL);

}

--

2.31.1