qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH RFC V2 2/9] vl: use qemu machine QOM class inste


From: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [PATCH RFC V2 2/9] vl: use qemu machine QOM class instead of global machines list
Date: Mon, 03 Mar 2014 21:54:51 +0200

On Mon, 2014-03-03 at 19:12 +0100, Andreas Färber wrote:
> 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?
Not at all

> 
> >  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.
Sure
> 
> > +{
> > +    QemuMachineClass *k = QEMU_MACHINE_CLASS(klass);
> 
> "mc" for [QEMU]MachineClass maybe? Applies elsewhere as well.
As above, I couldn't follow a unified naming scheme,
your comment really help, thanks!

> 
> On that matter, might we simply choose MachineState and MachineClass, or
> is there some name conflict?
As stated in your 1/9 patch review, I find your choice great,
indeed, there is no need for the "Qemu" prefix. It mostly helped *me*
to associate old-new names, but there is no naming conflict, I'll change.

> 
> > +
> > +    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!
Thanks!

> 
> 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().
I encountered no problems here (meaning it works), at least I see no race:
The [Qemu]MachineClasses are created dynamically after
'module_call_init(MODULE_INIT_QOM)' is called with no connection with what 
happened
there, beside the creation of the ObjectClass of course, which we can count it 
(can we?).
Other than that, we can say that a side effect of 
'module_call_init(MODULE_INIT_MACHINE)'
is the creation of the new Machine QOM classes.

Please let me know if I am missing something, in that case I'll switch as you 
suggested.
For the moment is how I see things: MODULE_INIT_QOM should be before 
MODULE_INIT_MACHINE.
 

> Otherwise the use of QOM infrastructure looks really nice.
Thanks!

If I am not exaggerating, can you please go over the patch 3/9 ?
([PATCH RFC V2 3/9] hw/boards: converted current_machine to be an instance of 
QemuMachineCLass)

I have a minor change to do there (add the machine to the QOM tree instead
of the /machine container) and combined with the first two, they can be a
nice feature start :) - and let me concentrate on the next phase.

Thanks again or your time!
Marcel



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






reply via email to

[Prev in Thread] Current Thread [Next in Thread]