From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57385) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKYww-0002Rh-Nn for qemu-devel@nongnu.org; Mon, 03 Mar 2014 14:55:00 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WKYwq-00083e-Cv for qemu-devel@nongnu.org; Mon, 03 Mar 2014 14:54:54 -0500 Received: from mx1.redhat.com ([209.132.183.28]:61247) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKYwq-00083W-4w for qemu-devel@nongnu.org; Mon, 03 Mar 2014 14:54:48 -0500 Message-ID: <1393876491.3882.72.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Mon, 03 Mar 2014 21:54:51 +0200 In-Reply-To: <5314C610.2090305@suse.de> References: <1393765632-2753-1-git-send-email-marcel.a@redhat.com> <1393765632-2753-3-git-send-email-marcel.a@redhat.com> <5314C610.2090305@suse.de> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH RFC V2 2/9] vl: use qemu machine QOM class instead of global machines list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?ISO-8859-1?Q?F=E4rber?= Cc: peter.maydell@linaro.org, peter.crosthwaite@petalogix.com, mdroth@linux.vnet.ibm.com, mst@redhat.com, rth@twiddle.net, mtosatti@redhat.com, edgar.iglesias@gmail.com, qemu-devel@nongnu.org, armbru@redhat.com, blauwirbel@gmail.com, quintela@redhat.com, agraf@suse.de, aliguori@amazon.com, pbonzini@redhat.com, scottwood@freescale.com, imammedo@redhat.com, lcapitulino@redhat.com, ehabkost@redhat.com On Mon, 2014-03-03 at 19:12 +0100, Andreas F=C3=A4rber wrote: > Am 02.03.2014 14:07, schrieb Marcel Apfelbaum: > > The machine registration flow is refactored to use the QOM functional= ity. > > Instead of linking the machines into a list, each machine has a type > > and the types can be traversed in the QOM way. > >=20 > > Signed-off-by: Marcel Apfelbaum > > --- > > include/hw/boards.h | 1 + > > vl.c | 75 ++++++++++++++++++++++++++++++++++++++-----= ---------- > > 2 files changed, 55 insertions(+), 21 deletions(-) > >=20 > > 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; > > }; > > =20 > > +#define TYPE_QEMU_MACHINE_PREFIX "machine-" >=20 > Would you mind turning that into a "-machine" suffix? Not at all >=20 > > int qemu_register_machine(QEMUMachine *m); > > QEMUMachine *find_default_machine(void); > > =20 > > 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 *q= dict) > > /***********************************************************/ > > /* machine registration */ > > =20 > > -static QEMUMachine *first_machine =3D NULL; > > QEMUMachine *current_machine =3D NULL; > > =20 > > +static void qemu_machine_class_init(ObjectClass *klass, void *data) >=20 > "oc" like in 1/9 please. Sure >=20 > > +{ > > + QemuMachineClass *k =3D QEMU_MACHINE_CLASS(klass); >=20 > "mc" for [QEMU]MachineClass maybe? Applies elsewhere as well. As above, I couldn't follow a unified naming scheme, your comment really help, thanks! >=20 > On that matter, might we simply choose MachineState and MachineClass, o= r > 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. >=20 > > + > > + k->qemu_machine =3D data; > > +} > > + > > int qemu_register_machine(QEMUMachine *m) > > { > > - QEMUMachine **pm; > > - pm =3D &first_machine; > > - while (*pm !=3D NULL) > > - pm =3D &(*pm)->next; > > - m->next =3D NULL; > > - *pm =3D m; > > + TypeInfo ti =3D { > > + .name =3D g_strconcat(TYPE_QEMU_MACHINE_PREFIX, m->nam= e, NULL), > > + .parent =3D TYPE_QEMU_MACHINE, > > + .class_init =3D qemu_machine_class_init, > > + .class_data =3D (void *)m, > > + }; > > + > > + type_register(&ti); >=20 > Cute idea as minimally invasive solution! Thanks! >=20 > 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 wha= t happened there, beside the creation of the ObjectClass of course, which we can cou= nt it (can we?). Other than that, we can say that a side effect of 'module_call_init(MODUL= E_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 MODU= LE_INIT_MACHINE. =20 > 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 instanc= e of QemuMachineCLass) I have a minor change to do there (add the machine to the QOM tree instea= d 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 >=20 > Cheers, > Andreas >=20 > > + > > return 0; > > } > > =20 > > static QEMUMachine *find_machine(const char *name) > > { > > - QEMUMachine *m; > > + GSList *el, *machines =3D object_class_get_list(TYPE_QEMU_MACHIN= E, false); > > + QEMUMachine *m =3D NULL; > > + > > + for (el =3D machines; el; el =3D el->next) { > > + QemuMachineClass *k =3D el->data; > > =20 > > - for(m =3D first_machine; m !=3D NULL; m =3D 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 =3D k->qemu_machine; > > + break; > > + } > > + if (k->qemu_machine->alias && !strcmp(k->qemu_machine->alias= , name)) { > > + m =3D k->qemu_machine; > > + break; > > + } > > } > > - return NULL; > > + > > + g_slist_free(machines); > > + return m; > > } > > =20 > > QEMUMachine *find_default_machine(void) > > { > > - QEMUMachine *m; > > + GSList *el, *machines =3D object_class_get_list(TYPE_QEMU_MACHIN= E, false); > > + QEMUMachine *m =3D NULL; > > =20 > > - for(m =3D first_machine; m !=3D NULL; m =3D m->next) { > > - if (m->is_default) { > > - return m; > > + for (el =3D machines; el; el =3D el->next) { > > + QemuMachineClass *k =3D el->data; > > + > > + if (k->qemu_machine->is_default) { > > + m =3D k->qemu_machine; > > + break; > > } > > } > > - return NULL; > > + > > + g_slist_free(machines); > > + return m; > > } > > =20 > > MachineInfoList *qmp_query_machines(Error **errp) > > { > > + GSList *el, *machines =3D object_class_get_list(TYPE_QEMU_MACHIN= E, false); > > MachineInfoList *mach_list =3D NULL; > > QEMUMachine *m; > > =20 > > - for (m =3D first_machine; m; m =3D m->next) { > > + for (el =3D machines; el; el =3D el->next) { > > + QemuMachineClass *k =3D el->data; > > MachineInfoList *entry; > > MachineInfo *info; > > =20 > > + m =3D k->qemu_machine; > > info =3D g_malloc0(sizeof(*info)); > > if (m->is_default) { > > info->has_is_default =3D true; > > @@ -1597,6 +1624,7 @@ MachineInfoList *qmp_query_machines(Error **err= p) > > mach_list =3D entry; > > } > > =20 > > + g_slist_free(machines); > > return mach_list; > > } > > =20 > > @@ -2540,6 +2568,7 @@ static int debugcon_parse(const char *devname) > > static QEMUMachine *machine_parse(const char *name) > > { > > QEMUMachine *m, *machine =3D NULL; > > + GSList *el, *machines =3D object_class_get_list(TYPE_QEMU_MACHIN= E, false); > > =20 > > if (name) { > > machine =3D find_machine(name); > > @@ -2548,13 +2577,17 @@ static QEMUMachine *machine_parse(const char = *name) > > return machine; > > } > > printf("Supported machines are:\n"); > > - for (m =3D first_machine; m !=3D NULL; m =3D m->next) { > > + for (el =3D machines; el; el =3D el->next) { > > + QemuMachineClass *k =3D el->data; > > + m =3D 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)); > > } > > =20 >=20