Re: [RFC V1 11/14] monitor: connect in precreate

[Peter Xu <peterx@redhat.com>]

On Thu, Oct 17, 2024 at 08:14:12AM -0700, Steve Sistare wrote:
> Complete monitor connections as early as possible, prior to
> qemu_create_early_backends, so the user can issue commands during the
> precreate phase.
> 
> Make a list of the chardev's referenced by all monitors.  Create the
> chardevs, then create the monitors.  Exclude monitor chardevs and
> monitors from the later creation phases.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  system/vl.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 77 insertions(+), 4 deletions(-)
> 
> diff --git a/system/vl.c b/system/vl.c
> index 3c592b9..a985ab8 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -1939,6 +1939,11 @@ static bool object_create_early(const ObjectOption *opt)
>          return false;
>      }
>  
> +    /* Reason: already created. */
> +    if (g_str_equal(type, "mon")) {
> +        return false;
> +    }

Why monitor are part of "object"s?  I thought it was only registered on
qemu_find_opts("mon").

Same question to object_create_late() below.

> +
>      return true;
>  }
>  
> @@ -1956,6 +1961,68 @@ static void qemu_apply_machine_options(QDict *qdict)
>      }
>  }
>  
> +typedef struct NamedElement {
> +    char *name;
> +    QTAILQ_ENTRY(NamedElement) next;
> +} NamedElement;
> +
> +static QTAILQ_HEAD(, NamedElement) monitor_chardevs =
> +    QTAILQ_HEAD_INITIALIZER(monitor_chardevs);
> +
> +static void chardev_add(const char *name)
> +{
> +    NamedElement *elem = g_new0(NamedElement, 1);
> +
> +    elem->name = g_strdup(name);
> +    QTAILQ_INSERT_TAIL(&monitor_chardevs, elem, next);
> +}
> +
> +static bool chardev_find(const char *name)
> +{
> +    NamedElement *elem;
> +
> +    QTAILQ_FOREACH(elem, &monitor_chardevs, next) {
> +        if (g_str_equal(elem->name, name)) {
> +            return true;
> +        }
> +    }
> +    return false;
> +}
> +
> +static int monitor_add_chardev(void *opaque, QemuOpts *opts, Error **errp)
> +{
> +    g_autofree char *chardev = NULL;
> +    int ret = monitor_chardev_name(opts, &chardev, errp);
> +
> +    if (!ret && chardev) {
> +        chardev_add(chardev);
> +    }
> +    return ret;
> +}
> +
> +static bool option_is_monitor_chardev(void *opaque, QemuOpts *opts)
> +{
> +    return chardev_find(qemu_opts_id(opts));
> +}
> +
> +static bool option_is_not_monitor_chardev(void *opaque, QemuOpts *opts)
> +{
> +    return !chardev_find(qemu_opts_id(opts));
> +}
> +
> +static void qemu_create_monitors(void)

Would be good to add some generic comment on why monitors' chardev can be
created earlier before the rest.

PS: I'm not yet sure this is required for the initial support for
cpr-transfer, as there's no chardev fds involved yet?  IOW, I am curious
what happens if this code init all chardevs instead of monitor-only.

> +{
> +    qemu_opts_foreach(qemu_find_opts("mon"),
> +                      monitor_add_chardev, NULL, &error_fatal);
> +
> +    qemu_opts_filter_foreach(qemu_find_opts("chardev"),
> +                      option_is_monitor_chardev,
> +                      chardev_init_func, NULL, &error_fatal);
> +
> +    qemu_opts_foreach(qemu_find_opts("mon"),
> +                      mon_init_func, NULL, &error_fatal);
> +}
> +
>  static void qemu_create_early_backends(void)
>  {
>      MachineClass *machine_class = MACHINE_GET_CLASS(current_machine);
> @@ -1994,7 +2061,8 @@ static void qemu_create_early_backends(void)
>      /* spice must initialize before chardevs (for spicevmc and spiceport) */
>      qemu_spice.init();
>  
> -    qemu_opts_foreach(qemu_find_opts("chardev"),
> +    qemu_opts_filter_foreach(qemu_find_opts("chardev"),
> +                      option_is_not_monitor_chardev,
>                        chardev_init_func, NULL, &error_fatal);
>  
>  #ifdef CONFIG_VIRTFS
> @@ -2020,6 +2088,11 @@ static void qemu_create_early_backends(void)
>   */
>  static bool object_create_late(const ObjectOption *opt)
>  {
> +    /* Reason: already created. */
> +    if (g_str_equal(ObjectType_str(opt->opts->qom_type), "mon")) {
> +        return false;
> +    }
> +
>      return !object_create_early(opt) && !object_create_pre_sandbox(opt);
>  }
>  
> @@ -2045,9 +2118,6 @@ static void qemu_create_late_backends(void)
>          exit(1);
>      }
>  
> -    qemu_opts_foreach(qemu_find_opts("mon"),
> -                      mon_init_func, NULL, &error_fatal);
> -
>      if (foreach_device_config(DEV_SERIAL, serial_parse) < 0)
>          exit(1);
>      if (foreach_device_config(DEV_PARALLEL, parallel_parse) < 0)
> @@ -3730,6 +3800,9 @@ void qemu_init(int argc, char **argv)
>  
>      accel = configure_accelerators(argv[0]);
>  
> +    os_setup_signal_handling();

Didn't immediately see why this line.  Some explanations / comments could
be helpful..

> +    qemu_create_monitors();
> +
>      /*
>       * QOM objects created after this point see all global and
>       * compat properties.
> -- 
> 1.8.3.1
> 

-- 
Peter Xu