Re: [PATCH v2 08/21] memory: Add Error** argument to .log_global*() handlers
Hello Peter,
I missed this email last week, so I might not have addressed your
comments. sorry about that.
On 2/29/24 06:29, Peter Xu wrote:
On Tue, Feb 27, 2024 at 07:03:32PM +0100, Cédric Le Goater wrote:
Modify all log_global*() handlers to take an Error** parameter and
return a bool. A new MEMORY_LISTENER_CALL_LOG_GLOBAL macro looping on
the listeners is introduced to handle a possible error, which will
would interrupt the loop if necessary.
To be noted a change in memory_global_dirty_log_start() behavior as it
will return as soon as an error is detected.
Cc: Stefano Stabellini
Cc: Anthony Perard
Cc: Paul Durrant
Cc: Michael S. Tsirkin
Cc: Paolo Bonzini
Cc: David Hildenbrand
Signed-off-by: Cédric Le Goater
---
include/exec/memory.h | 15 ++--
hw/i386/xen/xen-hvm.c | 6 ++--
hw/vfio/common.c | 8 +++--
hw/virtio/vhost.c | 6 ++--
system/memory.c | 83 +--
system/physmem.c | 5 +--
6 files changed, 101 insertions(+), 22 deletions(-)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index
8626a355b310ed7b1a1db7978ba4b394032c2f15..4bc146c5ebdd377cd14a4e462f32cc945db5a0a8
100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -998,8 +998,11 @@ struct MemoryListener {
* active at that time.
*
* @listener: The #MemoryListener.
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Return: true on success, else false setting @errp with error.
*/
-void (*log_global_start)(MemoryListener *listener);
+bool (*log_global_start)(MemoryListener *listener, Error **errp);
/**
* @log_global_stop:
@@ -1009,8 +1012,11 @@ struct MemoryListener {
* the address space.
*
* @listener: The #MemoryListener.
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Return: true on success, else false setting @errp with error.
*/
-void (*log_global_stop)(MemoryListener *listener);
+bool (*log_global_stop)(MemoryListener *listener, Error **errp);
/**
* @log_global_after_sync:
@@ -1019,8 +1025,11 @@ struct MemoryListener {
* for any #MemoryRegionSection.
*
* @listener: The #MemoryListener.
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Return: true on success, else false setting @errp with error.
*/
-void (*log_global_after_sync)(MemoryListener *listener);
+bool (*log_global_after_sync)(MemoryListener *listener, Error **errp);
/**
* @eventfd_add:
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index
f42621e6742552035122ea58092c91c3458338ff..925a207b494b4eed52d5f360b554f18ac8a9806d
100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -446,16 +446,18 @@ static void xen_log_sync(MemoryListener *listener,
MemoryRegionSection *section)
int128_get64(section->size));
}
-static void xen_log_global_start(MemoryListener *listener)
+static bool xen_log_global_start(MemoryListener *listener, Error **errp)
{
if (xen_enabled()) {
xen_in_migration = true;
}
+return true;
}
-static void xen_log_global_stop(MemoryListener *listener)
+static bool xen_log_global_stop(MemoryListener *listener, Error **errp)
{
xen_in_migration = false;
+return true;
}
static const MemoryListener xen_memory_listener = {
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index
059bfdc07a85e2eb908df828c1f42104d683e911..8bba95ba6a2010b78cae54c6905857686bbb6309
100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1075,7 +1075,8 @@ out:
return ret;
}
-static void vfio_listener_log_global_start(MemoryListener *listener)
+static bool vfio_listener_log_global_start(MemoryListener *listener,
+ Error **errp)
{
VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
listener);
@@ -1092,9 +1093,11 @@ static void
vfio_listener_log_global_start(MemoryListener *listener)
ret, strerror(-ret));
vfio_set_migration_error(ret);
}
+return !!ret;
}
-static void vfio_listener_log_global_stop(MemoryListener *listener)
+static bool vfio_listener_log_global_stop(MemoryListener *listener,
+ Error **errp)
{
VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
listener);
@@ -,6 +1114,7 @@ static void vfio_listener_log_global_stop(MemoryListener
*listener)
ret, strerror(-ret));
vfio_set_migration_error(ret);
}
+return !!ret;
}
Shouldn't both of these be "return !ret"?
Indeed. It went unnoticed because the return value is not tested when the
memory macros
Re: [PATCH v2 08/21] memory: Add Error** argument to .log_global*() handlers
On Tue, Feb 27, 2024 at 07:03:32PM +0100, Cédric Le Goater wrote:
> Modify all log_global*() handlers to take an Error** parameter and
> return a bool. A new MEMORY_LISTENER_CALL_LOG_GLOBAL macro looping on
> the listeners is introduced to handle a possible error, which will
> would interrupt the loop if necessary.
>
> To be noted a change in memory_global_dirty_log_start() behavior as it
> will return as soon as an error is detected.
>
> Cc: Stefano Stabellini
> Cc: Anthony Perard
> Cc: Paul Durrant
> Cc: Michael S. Tsirkin
> Cc: Paolo Bonzini
> Cc: David Hildenbrand
> Signed-off-by: Cédric Le Goater
> ---
> include/exec/memory.h | 15 ++--
> hw/i386/xen/xen-hvm.c | 6 ++--
> hw/vfio/common.c | 8 +++--
> hw/virtio/vhost.c | 6 ++--
> system/memory.c | 83 +--
> system/physmem.c | 5 +--
> 6 files changed, 101 insertions(+), 22 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index
> 8626a355b310ed7b1a1db7978ba4b394032c2f15..4bc146c5ebdd377cd14a4e462f32cc945db5a0a8
> 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -998,8 +998,11 @@ struct MemoryListener {
> * active at that time.
> *
> * @listener: The #MemoryListener.
> + * @errp: pointer to Error*, to store an error if it happens.
> + *
> + * Return: true on success, else false setting @errp with error.
> */
> -void (*log_global_start)(MemoryListener *listener);
> +bool (*log_global_start)(MemoryListener *listener, Error **errp);
>
> /**
> * @log_global_stop:
> @@ -1009,8 +1012,11 @@ struct MemoryListener {
> * the address space.
> *
> * @listener: The #MemoryListener.
> + * @errp: pointer to Error*, to store an error if it happens.
> + *
> + * Return: true on success, else false setting @errp with error.
> */
> -void (*log_global_stop)(MemoryListener *listener);
> +bool (*log_global_stop)(MemoryListener *listener, Error **errp);
>
> /**
> * @log_global_after_sync:
> @@ -1019,8 +1025,11 @@ struct MemoryListener {
> * for any #MemoryRegionSection.
> *
> * @listener: The #MemoryListener.
> + * @errp: pointer to Error*, to store an error if it happens.
> + *
> + * Return: true on success, else false setting @errp with error.
> */
> -void (*log_global_after_sync)(MemoryListener *listener);
> +bool (*log_global_after_sync)(MemoryListener *listener, Error **errp);
>
> /**
> * @eventfd_add:
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index
> f42621e6742552035122ea58092c91c3458338ff..925a207b494b4eed52d5f360b554f18ac8a9806d
> 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -446,16 +446,18 @@ static void xen_log_sync(MemoryListener *listener,
> MemoryRegionSection *section)
>int128_get64(section->size));
> }
>
> -static void xen_log_global_start(MemoryListener *listener)
> +static bool xen_log_global_start(MemoryListener *listener, Error **errp)
> {
> if (xen_enabled()) {
> xen_in_migration = true;
> }
> +return true;
> }
>
> -static void xen_log_global_stop(MemoryListener *listener)
> +static bool xen_log_global_stop(MemoryListener *listener, Error **errp)
> {
> xen_in_migration = false;
> +return true;
> }
>
> static const MemoryListener xen_memory_listener = {
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index
> 059bfdc07a85e2eb908df828c1f42104d683e911..8bba95ba6a2010b78cae54c6905857686bbb6309
> 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1075,7 +1075,8 @@ out:
> return ret;
> }
>
> -static void vfio_listener_log_global_start(MemoryListener *listener)
> +static bool vfio_listener_log_global_start(MemoryListener *listener,
> + Error **errp)
> {
> VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
> listener);
> @@ -1092,9 +1093,11 @@ static void
> vfio_listener_log_global_start(MemoryListener *listener)
> ret, strerror(-ret));
> vfio_set_migration_error(ret);
> }
> +return !!ret;
> }
>
> -static void vfio_listener_log_global_stop(MemoryListener *listener)
> +static bool vfio_listener_log_global_stop(MemoryListener *listener,
> + Error **errp)
> {
> VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
> listener);
> @@ -,6 +1114,7 @@ static void
> vfio_listener_log_global_stop(MemoryListener *listener)
> ret, strerror(-ret));
> vfio_set_migration_error(ret);
> }
> +return !!ret;
> }
Shouldn't both of these be "return !ret"?
Besides, maybe it'll be easier to go with either:
1) if to
[PATCH v2 08/21] memory: Add Error** argument to .log_global*() handlers
Modify all log_global*() handlers to take an Error** parameter and
return a bool. A new MEMORY_LISTENER_CALL_LOG_GLOBAL macro looping on
the listeners is introduced to handle a possible error, which will
would interrupt the loop if necessary.
To be noted a change in memory_global_dirty_log_start() behavior as it
will return as soon as an error is detected.
Cc: Stefano Stabellini
Cc: Anthony Perard
Cc: Paul Durrant
Cc: Michael S. Tsirkin
Cc: Paolo Bonzini
Cc: David Hildenbrand
Signed-off-by: Cédric Le Goater
---
include/exec/memory.h | 15 ++--
hw/i386/xen/xen-hvm.c | 6 ++--
hw/vfio/common.c | 8 +++--
hw/virtio/vhost.c | 6 ++--
system/memory.c | 83 +--
system/physmem.c | 5 +--
6 files changed, 101 insertions(+), 22 deletions(-)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index
8626a355b310ed7b1a1db7978ba4b394032c2f15..4bc146c5ebdd377cd14a4e462f32cc945db5a0a8
100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -998,8 +998,11 @@ struct MemoryListener {
* active at that time.
*
* @listener: The #MemoryListener.
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Return: true on success, else false setting @errp with error.
*/
-void (*log_global_start)(MemoryListener *listener);
+bool (*log_global_start)(MemoryListener *listener, Error **errp);
/**
* @log_global_stop:
@@ -1009,8 +1012,11 @@ struct MemoryListener {
* the address space.
*
* @listener: The #MemoryListener.
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Return: true on success, else false setting @errp with error.
*/
-void (*log_global_stop)(MemoryListener *listener);
+bool (*log_global_stop)(MemoryListener *listener, Error **errp);
/**
* @log_global_after_sync:
@@ -1019,8 +1025,11 @@ struct MemoryListener {
* for any #MemoryRegionSection.
*
* @listener: The #MemoryListener.
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Return: true on success, else false setting @errp with error.
*/
-void (*log_global_after_sync)(MemoryListener *listener);
+bool (*log_global_after_sync)(MemoryListener *listener, Error **errp);
/**
* @eventfd_add:
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index
f42621e6742552035122ea58092c91c3458338ff..925a207b494b4eed52d5f360b554f18ac8a9806d
100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -446,16 +446,18 @@ static void xen_log_sync(MemoryListener *listener,
MemoryRegionSection *section)
int128_get64(section->size));
}
-static void xen_log_global_start(MemoryListener *listener)
+static bool xen_log_global_start(MemoryListener *listener, Error **errp)
{
if (xen_enabled()) {
xen_in_migration = true;
}
+return true;
}
-static void xen_log_global_stop(MemoryListener *listener)
+static bool xen_log_global_stop(MemoryListener *listener, Error **errp)
{
xen_in_migration = false;
+return true;
}
static const MemoryListener xen_memory_listener = {
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index
059bfdc07a85e2eb908df828c1f42104d683e911..8bba95ba6a2010b78cae54c6905857686bbb6309
100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1075,7 +1075,8 @@ out:
return ret;
}
-static void vfio_listener_log_global_start(MemoryListener *listener)
+static bool vfio_listener_log_global_start(MemoryListener *listener,
+ Error **errp)
{
VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
listener);
@@ -1092,9 +1093,11 @@ static void
vfio_listener_log_global_start(MemoryListener *listener)
ret, strerror(-ret));
vfio_set_migration_error(ret);
}
+return !!ret;
}
-static void vfio_listener_log_global_stop(MemoryListener *listener)
+static bool vfio_listener_log_global_stop(MemoryListener *listener,
+ Error **errp)
{
VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
listener);
@@ -,6 +1114,7 @@ static void vfio_listener_log_global_stop(MemoryListener
*listener)
ret, strerror(-ret));
vfio_set_migration_error(ret);
}
+return !!ret;
}
static int vfio_device_dma_logging_report(VFIODevice *vbasedev, hwaddr iova,
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index
2c9ac794680ea9b65eba6cc22e70cf141e90aa73..7a555f941934991a72a2817e5505fe0ce6d6fc64
100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1044,7 +1044,7 @@ check_dev_state:
return r;
}
-static void vhost_log_global_start(MemoryListener *listener)
+static bool
