Re: [Qemu-devel] [PATCH RFC V2 2/9] vl: use qemu machine QOM class instead of global machines list

[Andreas Färber]

Am 02.03.2014 14:07, schrieb Marcel Apfelbaum:
> The machine registration flow is refactored to use the QOM functionality.
> Instead of linking the machines into a list, each machine has a type
> and the types can be traversed in the QOM way.
> 
> Signed-off-by: Marcel Apfelbaum <address@hidden>
> ---
>  include/hw/boards.h |  1 +
>  vl.c                | 75 
> ++++++++++++++++++++++++++++++++++++++---------------
>  2 files changed, 55 insertions(+), 21 deletions(-)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 7b4708d..65e1e03 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -49,6 +49,7 @@ struct QEMUMachine {
>      const char *hw_version;
>  };
>  
> +#define TYPE_QEMU_MACHINE_PREFIX "machine-"

Would you mind turning that into a "-machine" suffix?

>  int qemu_register_machine(QEMUMachine *m);
>  QEMUMachine *find_default_machine(void);
>  
> diff --git a/vl.c b/vl.c
> index 9379d33..50c880f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1529,54 +1529,81 @@ void pcmcia_info(Monitor *mon, const QDict *qdict)
>  /***********************************************************/
>  /* machine registration */
>  
> -static QEMUMachine *first_machine = NULL;
>  QEMUMachine *current_machine = NULL;
>  
> +static void qemu_machine_class_init(ObjectClass *klass, void *data)

"oc" like in 1/9 please.

> +{
> +    QemuMachineClass *k = QEMU_MACHINE_CLASS(klass);

"mc" for [QEMU]MachineClass maybe? Applies elsewhere as well.

On that matter, might we simply choose MachineState and MachineClass, or
is there some name conflict?

> +
> +    k->qemu_machine = data;
> +}
> +
>  int qemu_register_machine(QEMUMachine *m)
>  {
> -    QEMUMachine **pm;
> -    pm = &first_machine;
> -    while (*pm != NULL)
> -        pm = &(*pm)->next;
> -    m->next = NULL;
> -    *pm = m;
> +    TypeInfo ti = {
> +        .name       = g_strconcat(TYPE_QEMU_MACHINE_PREFIX, m->name, NULL),
> +        .parent     = TYPE_QEMU_MACHINE,
> +        .class_init = qemu_machine_class_init,
> +        .class_data = (void *)m,
> +    };
> +
> +    type_register(&ti);

Cute idea as minimally invasive solution!

I do wonder whether doing type_register() as part of machine_init()
callbacks could pose any timing problems - have you checked on that in
vl.c? I.e. in the worst case we might need to sweep
s/machine_init/type_init/ for all machines in this early patch already
while still calling qemu_register_machine().

Otherwise the use of QOM infrastructure looks really nice.

Cheers,
Andreas

> +
>      return 0;
>  }
>  
>  static QEMUMachine *find_machine(const char *name)
>  {
> -    QEMUMachine *m;
> +    GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
> +    QEMUMachine *m = NULL;
> +
> +    for (el = machines; el; el = el->next) {
> +        QemuMachineClass *k = el->data;
>  
> -    for(m = first_machine; m != NULL; m = m->next) {
> -        if (!strcmp(m->name, name))
> -            return m;
> -        if (m->alias && !strcmp(m->alias, name))
> -            return m;
> +        if (!strcmp(k->qemu_machine->name, name)) {
> +            m = k->qemu_machine;
> +            break;
> +        }
> +        if (k->qemu_machine->alias && !strcmp(k->qemu_machine->alias, name)) 
> {
> +            m = k->qemu_machine;
> +            break;
> +        }
>      }
> -    return NULL;
> +
> +    g_slist_free(machines);
> +    return m;
>  }
>  
>  QEMUMachine *find_default_machine(void)
>  {
> -    QEMUMachine *m;
> +    GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
> +    QEMUMachine *m = NULL;
>  
> -    for(m = first_machine; m != NULL; m = m->next) {
> -        if (m->is_default) {
> -            return m;
> +    for (el = machines; el; el = el->next) {
> +        QemuMachineClass *k = el->data;
> +
> +        if (k->qemu_machine->is_default) {
> +            m = k->qemu_machine;
> +            break;
>          }
>      }
> -    return NULL;
> +
> +    g_slist_free(machines);
> +    return m;
>  }
>  
>  MachineInfoList *qmp_query_machines(Error **errp)
>  {
> +    GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
>      MachineInfoList *mach_list = NULL;
>      QEMUMachine *m;
>  
> -    for (m = first_machine; m; m = m->next) {
> +    for (el = machines; el; el = el->next) {
> +        QemuMachineClass *k = el->data;
>          MachineInfoList *entry;
>          MachineInfo *info;
>  
> +        m = k->qemu_machine;
>          info = g_malloc0(sizeof(*info));
>          if (m->is_default) {
>              info->has_is_default = true;
> @@ -1597,6 +1624,7 @@ MachineInfoList *qmp_query_machines(Error **errp)
>          mach_list = entry;
>      }
>  
> +    g_slist_free(machines);
>      return mach_list;
>  }
>  
> @@ -2540,6 +2568,7 @@ static int debugcon_parse(const char *devname)
>  static QEMUMachine *machine_parse(const char *name)
>  {
>      QEMUMachine *m, *machine = NULL;
> +    GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
>  
>      if (name) {
>          machine = find_machine(name);
> @@ -2548,13 +2577,17 @@ static QEMUMachine *machine_parse(const char *name)
>          return machine;
>      }
>      printf("Supported machines are:\n");
> -    for (m = first_machine; m != NULL; m = m->next) {
> +    for (el = machines; el; el = el->next) {
> +        QemuMachineClass *k = el->data;
> +        m = k->qemu_machine;
>          if (m->alias) {
>              printf("%-20s %s (alias of %s)\n", m->alias, m->desc, m->name);
>          }
>          printf("%-20s %s%s\n", m->name, m->desc,
>                 m->is_default ? " (default)" : "");
>      }
> +
> +    g_slist_free(machines);
>      exit(!name || !is_help_option(name));
>  }
>  

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg