Re: [PATCH v2 1/2] spice: simplify chardev setup

[Daniel P . Berrangé]

On Wed, Sep 16, 2020 at 10:39:12AM +0200, Gerd Hoffmann wrote:
> Initialize spice before chardevs.  That allows to register the spice
> chardevs directly in the init function and removes the need to maintain
> a linked list of chardevs just for registration.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/chardev/spice.h |  1 -
>  include/ui/qemu-spice.h |  1 -
>  chardev/spice.c         | 29 ++++++-----------------------
>  softmmu/vl.c            |  9 +++++----
>  ui/spice-app.c          | 17 +++++++++--------
>  ui/spice-core.c         |  2 --
>  6 files changed, 20 insertions(+), 39 deletions(-)
> 
> diff --git a/include/chardev/spice.h b/include/chardev/spice.h
> index 99f26aedde54..543b93d38ce3 100644
> --- a/include/chardev/spice.h
> +++ b/include/chardev/spice.h
> @@ -13,7 +13,6 @@ struct SpiceChardev {
>      bool                  blocked;
>      const uint8_t         *datapos;
>      int                   datalen;
> -    QLIST_ENTRY(SpiceChardev) next;
>  };
>  typedef struct SpiceChardev SpiceChardev;
>  
> diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
> index 8c23dfe71797..d34cea2e0fcd 100644
> --- a/include/ui/qemu-spice.h
> +++ b/include/ui/qemu-spice.h
> @@ -46,7 +46,6 @@ int qemu_spice_migrate_info(const char *hostname, int port, 
> int tls_port,
>  #else
>  #define SPICE_NEEDS_SET_MM_TIME 0
>  #endif
> -void qemu_spice_register_ports(void);
>  
>  #else  /* CONFIG_SPICE */
>  
> diff --git a/chardev/spice.c b/chardev/spice.c
> index bf7ea1e2940d..9733f0671699 100644
> --- a/chardev/spice.c
> +++ b/chardev/spice.c
> @@ -14,9 +14,6 @@ typedef struct SpiceCharSource {
>      SpiceChardev       *scd;
>  } SpiceCharSource;
>  
> -static QLIST_HEAD(, SpiceChardev) spice_chars =
> -    QLIST_HEAD_INITIALIZER(spice_chars);
> -
>  static int vmc_write(SpiceCharDeviceInstance *sin, const uint8_t *buf, int 
> len)
>  {
>      SpiceChardev *scd = container_of(sin, SpiceChardev, sin);
> @@ -216,8 +213,6 @@ static void char_spice_finalize(Object *obj)
>  
>      vmc_unregister_interface(s);
>  
> -    QLIST_SAFE_REMOVE(s, next);
> -
>      g_free((char *)s->sin.subtype);
>      g_free((char *)s->sin.portname);
>  }
> @@ -256,8 +251,6 @@ static void chr_open(Chardev *chr, const char *subtype)
>  
>      s->active = false;
>      s->sin.subtype = g_strdup(subtype);
> -
> -    QLIST_INSERT_HEAD(&spice_chars, s, next);
>  }
>  
>  static void qemu_chr_open_spice_vmc(Chardev *chr,
> @@ -310,28 +303,18 @@ void qemu_chr_open_spice_port(Chardev *chr,
>          return;
>      }
>  
> +    if (!using_spice) {
> +        error_setg(errp, "spice not enabled");
> +        return;
> +    }
> +
>      chr_open(chr, "port");
>  
>      *be_opened = false;
>      s = SPICE_CHARDEV(chr);
>      s->sin.portname = g_strdup(name);
>  
> -    if (using_spice) {
> -        /* spice server already created */
> -        vmc_register_interface(s);
> -    }
> -}
> -
> -void qemu_spice_register_ports(void)
> -{
> -    SpiceChardev *s;
> -
> -    QLIST_FOREACH(s, &spice_chars, next) {
> -        if (s->sin.portname == NULL) {
> -            continue;
> -        }
> -        vmc_register_interface(s);
> -    }
> +    vmc_register_interface(s);
>  }
>  
>  static void qemu_chr_parse_spice_vmc(QemuOpts *opts, ChardevBackend *backend,
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index f7b103467c02..062468d76178 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -4121,6 +4121,11 @@ void qemu_init(int argc, char **argv, char **envp)
>      page_size_init();
>      socket_init();
>  
> +    /* spice needs the timers to be initialized by this point */
> +    /* spice must initialize before audio as it changes the default auiodev 
> */
> +    /* spice must initialize before chardevs (for spicevmc and spiceport) */
> +    qemu_spice_init();
> +

I don't think we should be putting this before the '-object' processing too.
We really want -object to be the first thing processed in general.

>      qemu_opts_foreach(qemu_find_opts("object"),
>                        user_creatable_add_opts_foreach,
>                        object_create_initial, &error_fatal);
> @@ -4133,10 +4138,6 @@ void qemu_init(int argc, char **argv, char **envp)
>                        fsdev_init_func, NULL, &error_fatal);
>  #endif
>  
> -    /* spice needs the timers to be initialized by this point */
> -    /* spice must initialize before audio as it changes the default auiodev 
> */
> -    qemu_spice_init();
> -
>      /*
>       * Note: we need to create audio and block backends before
>       * machine_set_property(), so machine properties can refer to

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|