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

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(-)

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
>

On 10/17/24 17:14, 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;
> +    }

This is incorrect as mentioned by Peter.

>       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;
> +}

No new special casing and no tricky differentiation of how a single 
command line option is processed.  If you need to create monitors so 
early, create _all_ chardevs and _all_ monitors; same for qtest.

Paolo

On 10/21/2024 3:28 PM, Peter Xu wrote:
> 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.

Thanks, my mistake, I'll delete it in both places.

>> +
>>       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.

Sure, for now I will simplify this and init all chardevs early, and add
the filter in the future series when CPR preserves chardevs.

>> +{
>> +    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..

The TERM handler must be installed before the qtest monitor connects, to
catch failure during precreate.  Previously it was not installed until
qemu_init_displays -> os_setup_signal_handling.  The latter is still called,
because of the comment "must be after terminal init, SDL library changes
signal handlers".  It is harmless to call os_setup_signal_handling twice.

- Steve

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

On 10/23/2024 12:05 PM, Paolo Bonzini wrote:
> On 10/17/24 17:14, 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;
>> +    }
> 
> This is incorrect as mentioned by Peter.

Got it.

>>       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;
>> +}
> 
> No new special casing and no tricky differentiation of how a single command line option is processed.  If you need to create monitors so early, create _all_ chardevs and _all_ monitors; same for qtest.

I do create all monitors.  But for chardevs, I only create those needed by the monitors
and qtest. That is so CPR can be used to preserve chardevs in a future patch.  But I will
defer that functionality to simplify this series, and create all chardevs early.

But hey, you have to admit the filtering patches are pretty spiffy :)

- Steve