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 <marce...@redhat.com>
> > ---
> > 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));
> > }
> >
>
