Re: [Qemu-devel] [PATCH] char: do not use atexit cleanup handler

[Daniel P. Berrange]

On Mon, Jul 04, 2016 at 05:38:23PM +0200, address@hidden wrote:
> From: Marc-André Lureau <address@hidden>
> 
> It turns out qemu is calling exit() in various places from various
> threads without taking much care of resources state. The atexit()
> cleanup handlers cannot easily destroy resources that are in use (by
> the same thread or other).

[snip]

> Instead of using a atexit() handler, only run the chardev cleanup as
> initially proposed at the end of main(), where there are less chances
> (hic) of conflicts or other races.

This doesn't really seem all that much safer. There's still plenty of
chance that threads are running in the background at the end of the
main() method, so plenty of scope for the qemu_chr_cleanup() call to
cause threads to segv by destroying the chardevs they're using behind
their back.

IIUC, the original intent here was that we call unlink() on the UNIX
socket paths when QEMU exits.

Surely we can come up with a way to that, and only that, upon exit,
without actually having to free the chardev memory with all the risks
that entails.

eg, have a qemu_chr_close() method that closes & cleans up resources,
separately from actually free'ing the Chardev struct with all the
risk of crashing concurrent threads that entails.

> Signed-off-by: Marc-André Lureau <address@hidden>
> Reported-by: Andrew Jones <address@hidden>
> ---
>  include/sysemu/char.h | 7 +++++++
>  qemu-char.c           | 4 +---
>  vl.c                  | 1 +
>  3 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index 57df10a..0ea9eac 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -151,6 +151,13 @@ CharDriverState *qemu_chr_new(const char *label, const 
> char *filename,
>   */
>  void qemu_chr_disconnect(CharDriverState *chr);
>  
> +/**
> + * @qemu_chr_cleanup:
> + *
> + * Delete all chardevs (when leaving qemu)
> + */
> +void qemu_chr_cleanup(void);
> +
>  /**
>   * @qemu_chr_new_noreplay:
>   *
> diff --git a/qemu-char.c b/qemu-char.c
> index b73969d..a542192 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -4549,7 +4549,7 @@ void qmp_chardev_remove(const char *id, Error **errp)
>      qemu_chr_delete(chr);
>  }
>  
> -static void qemu_chr_cleanup(void)
> +void qemu_chr_cleanup(void)
>  {
>      CharDriverState *chr, *tmp;
>  
> @@ -4604,8 +4604,6 @@ static void register_types(void)
>       * is specified
>       */
>      qemu_add_machine_init_done_notifier(&muxes_realize_notify);
> -
> -    atexit(qemu_chr_cleanup);
>  }
>  
>  type_init(register_types);
> diff --git a/vl.c b/vl.c
> index 9bb7f4c..d0b9ff9 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4596,6 +4596,7 @@ int main(int argc, char **argv, char **envp)
>  #ifdef CONFIG_TPM
>      tpm_cleanup();
>  #endif
> +    qemu_chr_cleanup();
>  
>      return 0;

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|