[PATCH 0/2] Do not abort on log-start/stop errors

[PATCH 0/2] Do not abort on log-start/stop errors

[Hanna Czenczek]

Hi,

vhost_log_global_start() and vhost_log_global_stop() abort the whole
qemu process on error.  Not least because vhost devices are generally
outside of qemu (i.e. use a foreign code base), that is not great, as we
can basically be forced to abort because of bugs (or maybe even properly
behaving, just unexpectedly so) in other code bases.

In case of vhost_log_global_start(), the solution is simple: Ever since
commit 3688fec8923 ("memory: Add Error** argument to .log_global_start()
handler"), it can just return proper errors, so do that instead of
aborting.

In case of vhost_log_global_stop(), we cannot return errors; but we can
just ignore them.  In the worst case, some other process will keep
logging into shared memory we have already unmapped.  That's fine.


Hanna Czenczek (2):
  vhost: Do not abort on log-start error
  vhost: Do not abort on log-stop error

 hw/virtio/vhost.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

-- 
2.50.1

[PATCH 1/2] vhost: Do not abort on log-start error

[Hanna Czenczek]

Commit 3688fec8923 ("memory: Add Error** argument to .log_global_start()
handler") enabled vhost_log_global_start() to return a proper error, but
did not change it to do so; instead, it still aborts the whole process
on error.

This crash can be reproduced by e.g. killing a virtiofsd daemon before
initiating migration.  In such a case, qemu should not crash, but just
make the attempted migration fail.

Buglink: https://issues.redhat.com/browse/RHEL-94534
Reported-by: Tingting Mao <timao@redhat.com>
Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
---
 hw/virtio/vhost.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index c30ea1156e..05ad5de629 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1110,7 +1110,8 @@ static bool vhost_log_global_start(MemoryListener *listener, Error **errp)
 
     r = vhost_migration_log(listener, true);
     if (r < 0) {
-        abort();
+        error_setg_errno(errp, -r, "vhost: Failed to start logging");
+        return false;
     }
     return true;
 }
-- 
2.50.1

Re: [PATCH 1/2] vhost: Do not abort on log-start error

[Manos Pitsidianakis]

On Thu, Jul 24, 2025 at 4:00 PM Hanna Czenczek <hreitz@redhat.com> wrote:
>
> Commit 3688fec8923 ("memory: Add Error** argument to .log_global_start()
> handler") enabled vhost_log_global_start() to return a proper error, but
> did not change it to do so; instead, it still aborts the whole process
> on error.
>
> This crash can be reproduced by e.g. killing a virtiofsd daemon before
> initiating migration.  In such a case, qemu should not crash, but just
> make the attempted migration fail.
>
> Buglink: https://issues.redhat.com/browse/RHEL-94534
> Reported-by: Tingting Mao <timao@redhat.com>
> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
> ---
>  hw/virtio/vhost.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index c30ea1156e..05ad5de629 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1110,7 +1110,8 @@ static bool vhost_log_global_start(MemoryListener *listener, Error **errp)
>
>      r = vhost_migration_log(listener, true);
>      if (r < 0) {
> -        abort();
> +        error_setg_errno(errp, -r, "vhost: Failed to start logging");
> +        return false;
>      }
>      return true;
>  }
> --
> 2.50.1
>
>

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

Re: [PATCH 1/2] vhost: Do not abort on log-start error

[Stefano Garzarella]

On Thu, Jul 24, 2025 at 02:59:27PM +0200, Hanna Czenczek wrote:
>Commit 3688fec8923 ("memory: Add Error** argument to .log_global_start()
>handler") enabled vhost_log_global_start() to return a proper error, but
>did not change it to do so; instead, it still aborts the whole process
>on error.
>
>This crash can be reproduced by e.g. killing a virtiofsd daemon before
>initiating migration.  In such a case, qemu should not crash, but just
>make the attempted migration fail.
>
>Buglink: https://issues.redhat.com/browse/RHEL-94534
>Reported-by: Tingting Mao <timao@redhat.com>
>Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
>---
> hw/virtio/vhost.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)

IIUC we always had the problem, so it's not a regression, but should we 
queue the patch in stable as well?

Anyway, it LGTM!

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

>
>diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>index c30ea1156e..05ad5de629 100644
>--- a/hw/virtio/vhost.c
>+++ b/hw/virtio/vhost.c
>@@ -1110,7 +1110,8 @@ static bool vhost_log_global_start(MemoryListener *listener, Error **errp)
>
>     r = vhost_migration_log(listener, true);
>     if (r < 0) {
>-        abort();
>+        error_setg_errno(errp, -r, "vhost: Failed to start logging");
>+        return false;
>     }
>     return true;
> }
>-- 
>2.50.1
>

Re: [PATCH 1/2] vhost: Do not abort on log-start error

[Hanna Czenczek]

On 24.07.25 16:24, Stefano Garzarella wrote:
> On Thu, Jul 24, 2025 at 02:59:27PM +0200, Hanna Czenczek wrote:
>> Commit 3688fec8923 ("memory: Add Error** argument to .log_global_start()
>> handler") enabled vhost_log_global_start() to return a proper error, but
>> did not change it to do so; instead, it still aborts the whole process
>> on error.
>>
>> This crash can be reproduced by e.g. killing a virtiofsd daemon before
>> initiating migration.  In such a case, qemu should not crash, but just
>> make the attempted migration fail.
>>
>> Buglink: https://issues.redhat.com/browse/RHEL-94534
>> Reported-by: Tingting Mao <timao@redhat.com>
>> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
>> ---
>> hw/virtio/vhost.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> IIUC we always had the problem, so it's not a regression, but should 
> we queue the patch in stable as well?

That’s my impression as well.  I think it fits and makes sense for 
stable, but it isn’t absolutely necessary; it’s not a regression, and 
abort()-ing is not a critical problem.

> Anyway, it LGTM!
>
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks!

Hanna

>
>>
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index c30ea1156e..05ad5de629 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -1110,7 +1110,8 @@ static bool 
>> vhost_log_global_start(MemoryListener *listener, Error **errp)
>>
>>     r = vhost_migration_log(listener, true);
>>     if (r < 0) {
>> -        abort();
>> +        error_setg_errno(errp, -r, "vhost: Failed to start logging");
>> +        return false;
>>     }
>>     return true;
>> }
>> -- 
>> 2.50.1
>>
>

[PATCH 2/2] vhost: Do not abort on log-stop error

[Hanna Czenczek]

Failing to stop logging in a vhost device is not exactly fatal.  We can
log such an error, but there is no need to abort the whole qemu process
because of it.

Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
---
 hw/virtio/vhost.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 05ad5de629..6557c58d12 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1122,7 +1122,8 @@ static void vhost_log_global_stop(MemoryListener *listener)
 
     r = vhost_migration_log(listener, false);
     if (r < 0) {
-        abort();
+        /* Not fatal, so report it, but take no further action */
+        warn_report("vhost: Failed to stop logging");
     }
 }
 
-- 
2.50.1

Re: [PATCH 2/2] vhost: Do not abort on log-stop error

[Manos Pitsidianakis]

On Thu, Jul 24, 2025 at 4:00 PM Hanna Czenczek <hreitz@redhat.com> wrote:
>
> Failing to stop logging in a vhost device is not exactly fatal.  We can
> log such an error, but there is no need to abort the whole qemu process
> because of it.
>
> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
> ---
>  hw/virtio/vhost.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 05ad5de629..6557c58d12 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1122,7 +1122,8 @@ static void vhost_log_global_stop(MemoryListener *listener)
>
>      r = vhost_migration_log(listener, false);
>      if (r < 0) {
> -        abort();
> +        /* Not fatal, so report it, but take no further action */
> +        warn_report("vhost: Failed to stop logging");
>      }
>  }
>
> --
> 2.50.1
>
>

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

-- 
Manos Pitsidianakis
Emulation and Virtualization Engineer at Linaro Ltd

Re: [PATCH 2/2] vhost: Do not abort on log-stop error

[Stefano Garzarella]

On Thu, Jul 24, 2025 at 02:59:28PM +0200, Hanna Czenczek wrote:
>Failing to stop logging in a vhost device is not exactly fatal.  We can
>log such an error, but there is no need to abort the whole qemu process
>because of it.
>
>Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
>---
> hw/virtio/vhost.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)

Ditto about stable here as well.

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

>
>diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>index 05ad5de629..6557c58d12 100644
>--- a/hw/virtio/vhost.c
>+++ b/hw/virtio/vhost.c
>@@ -1122,7 +1122,8 @@ static void vhost_log_global_stop(MemoryListener *listener)
>
>     r = vhost_migration_log(listener, false);
>     if (r < 0) {
>-        abort();
>+        /* Not fatal, so report it, but take no further action */
>+        warn_report("vhost: Failed to stop logging");
>     }
> }
>
>-- 
>2.50.1
>

Re: [PATCH 0/2] Do not abort on log-start/stop errors

[Lei Yang]

Tested this series of patches with vhost-net regression tests,
everything works fine.

Tested-by: Lei Yang <leiyang@redhat.com>

On Thu, Jul 24, 2025 at 9:01 PM Hanna Czenczek <hreitz@redhat.com> wrote:
>
> Hi,
>
> vhost_log_global_start() and vhost_log_global_stop() abort the whole
> qemu process on error.  Not least because vhost devices are generally
> outside of qemu (i.e. use a foreign code base), that is not great, as we
> can basically be forced to abort because of bugs (or maybe even properly
> behaving, just unexpectedly so) in other code bases.
>
> In case of vhost_log_global_start(), the solution is simple: Ever since
> commit 3688fec8923 ("memory: Add Error** argument to .log_global_start()
> handler"), it can just return proper errors, so do that instead of
> aborting.
>
> In case of vhost_log_global_stop(), we cannot return errors; but we can
> just ignore them.  In the worst case, some other process will keep
> logging into shared memory we have already unmapped.  That's fine.
>
>
> Hanna Czenczek (2):
>   vhost: Do not abort on log-start error
>   vhost: Do not abort on log-stop error
>
>  hw/virtio/vhost.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> --
> 2.50.1
>
>

Re: [PATCH 0/2] Do not abort on log-start/stop errors

[Michael Tokarev]

On 24.07.2025 15:59, Hanna Czenczek wrote:
> Hi,
> 
> vhost_log_global_start() and vhost_log_global_stop() abort the whole
> qemu process on error.  Not least because vhost devices are generally
> outside of qemu (i.e. use a foreign code base), that is not great, as we
> can basically be forced to abort because of bugs (or maybe even properly
> behaving, just unexpectedly so) in other code bases.
> 
> In case of vhost_log_global_start(), the solution is simple: Ever since
> commit 3688fec8923 ("memory: Add Error** argument to .log_global_start()
> handler"), it can just return proper errors, so do that instead of
> aborting.
> 
> In case of vhost_log_global_stop(), we cannot return errors; but we can
> just ignore them.  In the worst case, some other process will keep
> logging into shared memory we have already unmapped.  That's fine.
> 
> 
> Hanna Czenczek (2):
>    vhost: Do not abort on log-start error
>    vhost: Do not abort on log-stop error

I'm picking this up for qemu 10.0.x stable series, as simple
bugfixes.  Please let me know if I should not.

Thanks,

/mjt