diff mbox

[V2,2/5] machine: remove QEMUMachine indirection from MachineClass

Message ID 1396257993-4036-3-git-send-email-marcel.a@redhat.com
State New
Headers show

Commit Message

Marcel Apfelbaum March 31, 2014, 9:26 a.m. UTC 
No need to go through qemu_machine field. Use
MachineClass fields directly.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 device-hotplug.c |   2 +-
 qmp.c            |   4 +--
 vl.c             | 103 ++++++++++++++++++++++++++++++++-----------------------
 3 files changed, 63 insertions(+), 46 deletions(-)

Comments

Andreas Färber April 3, 2014, 4:59 p.m. UTC | #1 
Am 31.03.2014 11:26, schrieb Marcel Apfelbaum:
> No need to go through qemu_machine field. Use
> MachineClass fields directly.
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  device-hotplug.c |   2 +-
>  qmp.c            |   4 +--
>  vl.c             | 103 ++++++++++++++++++++++++++++++++-----------------------
>  3 files changed, 63 insertions(+), 46 deletions(-)
[...]
> diff --git a/vl.c b/vl.c
> index 9975e5a..96155ca 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1583,8 +1583,29 @@ MachineState *current_machine;
>  static void machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> -
> -    mc->qemu_machine = data;

This looks incomplete. You should drop the qemu_machine field from
boards.h to assure that no one is using it - pseries still is, so 4/5
needs to be be squashed into this commit, otherwise the new fields below
remain empty for it.

Regards,
Andreas

> +    QEMUMachine *qm = data;
> +
> +    mc->name = qm->name;
> +    mc->alias = qm->alias;
> +    mc->desc = qm->desc;
> +    mc->init = qm->init;
> +    mc->reset = qm->reset;
> +    mc->hot_add_cpu = qm->hot_add_cpu;
> +    mc->kvm_type = qm->kvm_type;
> +    mc->block_default_type = qm->block_default_type;
> +    mc->max_cpus = qm->max_cpus;
> +    mc->no_serial = qm->no_serial;
> +    mc->no_parallel = qm->no_parallel;
> +    mc->use_virtcon = qm->use_virtcon;
> +    mc->use_sclp = qm->use_sclp;
> +    mc->no_floppy = qm->no_floppy;
> +    mc->no_cdrom = qm->no_cdrom;
> +    mc->no_sdcard = qm->no_sdcard;
> +    mc->is_default = qm->is_default;
> +    mc->default_machine_opts = qm->default_machine_opts;
> +    mc->default_boot_order = qm->default_boot_order;
> +    mc->compat_props = qm->compat_props;
> +    mc->hw_version = qm->hw_version;
>  }
>  
>  int qemu_register_machine(QEMUMachine *m)
Marcel Apfelbaum April 3, 2014, 5:11 p.m. UTC | #2 
On Thu, 2014-04-03 at 18:59 +0200, Andreas Färber wrote:
> Am 31.03.2014 11:26, schrieb Marcel Apfelbaum:
> > No need to go through qemu_machine field. Use
> > MachineClass fields directly.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  device-hotplug.c |   2 +-
> >  qmp.c            |   4 +--
> >  vl.c             | 103 ++++++++++++++++++++++++++++++++-----------------------
> >  3 files changed, 63 insertions(+), 46 deletions(-)
> [...]
> > diff --git a/vl.c b/vl.c
> > index 9975e5a..96155ca 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1583,8 +1583,29 @@ MachineState *current_machine;
> >  static void machine_class_init(ObjectClass *oc, void *data)
> >  {
> >      MachineClass *mc = MACHINE_CLASS(oc);
> > -
> > -    mc->qemu_machine = data;
> 
> This looks incomplete. You should drop the qemu_machine field from
> boards.h to assure that no one is using it - pseries still is, so 4/5
> needs to be be squashed into this commit, otherwise the new fields below
> remain empty for it.
Yes, indeed, otherwise it will break bisection.
I was looking for a way to separate the changes for hw/ppc/spapr.c
into a separate patch.

Maybe I will leave both assignments (of the qemu_machine pointer and the fields)
in this patch and remove it on 5/5 (together with qemu_machine field from 
boards.h), leaving the 4/5 as it is?

Thanks,
Marcel 


> 
> Regards,
> Andreas
> 
> > +    QEMUMachine *qm = data;
> > +
> > +    mc->name = qm->name;
> > +    mc->alias = qm->alias;
> > +    mc->desc = qm->desc;
> > +    mc->init = qm->init;
> > +    mc->reset = qm->reset;
> > +    mc->hot_add_cpu = qm->hot_add_cpu;
> > +    mc->kvm_type = qm->kvm_type;
> > +    mc->block_default_type = qm->block_default_type;
> > +    mc->max_cpus = qm->max_cpus;
> > +    mc->no_serial = qm->no_serial;
> > +    mc->no_parallel = qm->no_parallel;
> > +    mc->use_virtcon = qm->use_virtcon;
> > +    mc->use_sclp = qm->use_sclp;
> > +    mc->no_floppy = qm->no_floppy;
> > +    mc->no_cdrom = qm->no_cdrom;
> > +    mc->no_sdcard = qm->no_sdcard;
> > +    mc->is_default = qm->is_default;
> > +    mc->default_machine_opts = qm->default_machine_opts;
> > +    mc->default_boot_order = qm->default_boot_order;
> > +    mc->compat_props = qm->compat_props;
> > +    mc->hw_version = qm->hw_version;
> >  }
> >  
> >  int qemu_register_machine(QEMUMachine *m)
>
Andreas Färber April 3, 2014, 5:36 p.m. UTC | #3 
Am 03.04.2014 19:11, schrieb Marcel Apfelbaum:
> On Thu, 2014-04-03 at 18:59 +0200, Andreas Färber wrote:
>> Am 31.03.2014 11:26, schrieb Marcel Apfelbaum:
>>> diff --git a/vl.c b/vl.c
>>> index 9975e5a..96155ca 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -1583,8 +1583,29 @@ MachineState *current_machine;
>>>  static void machine_class_init(ObjectClass *oc, void *data)
>>>  {
>>>      MachineClass *mc = MACHINE_CLASS(oc);
>>> -
>>> -    mc->qemu_machine = data;
>>
>> This looks incomplete. You should drop the qemu_machine field from
>> boards.h to assure that no one is using it - pseries still is, so 4/5
>> needs to be be squashed into this commit, otherwise the new fields below
>> remain empty for it.
> Yes, indeed, otherwise it will break bisection.
> I was looking for a way to separate the changes for hw/ppc/spapr.c
> into a separate patch.
> 
> Maybe I will leave both assignments (of the qemu_machine pointer and the fields)
> in this patch and remove it on 5/5 (together with qemu_machine field from 
> boards.h), leaving the 4/5 as it is?

The trouble with 2/5 is that in generic code you start using mc->
fields, which do not get assigned by pseries. So the "mc->... = ...;"
parts of 4/5 need to go into 2/5, whether you leave qemu_machine
assigned or not.

Of course I understand your desire to not put everything into one huge
commit. Maybe reordering 3/5 before this patch using ->qemu_machine
helps? Next prepare QEMUMachineInitArgs for MachineClass (5/5). Then in
this step you can drop qemu_machine field as suggested and any
->qemu_machine gets dropped in one big but mechanical change.

Regards,
Andreas
Marcel Apfelbaum April 3, 2014, 5:40 p.m. UTC | #4 
On Thu, 2014-04-03 at 19:36 +0200, Andreas Färber wrote:
> Am 03.04.2014 19:11, schrieb Marcel Apfelbaum:
> > On Thu, 2014-04-03 at 18:59 +0200, Andreas Färber wrote:
> >> Am 31.03.2014 11:26, schrieb Marcel Apfelbaum:
> >>> diff --git a/vl.c b/vl.c
> >>> index 9975e5a..96155ca 100644
> >>> --- a/vl.c
> >>> +++ b/vl.c
> >>> @@ -1583,8 +1583,29 @@ MachineState *current_machine;
> >>>  static void machine_class_init(ObjectClass *oc, void *data)
> >>>  {
> >>>      MachineClass *mc = MACHINE_CLASS(oc);
> >>> -
> >>> -    mc->qemu_machine = data;
> >>
> >> This looks incomplete. You should drop the qemu_machine field from
> >> boards.h to assure that no one is using it - pseries still is, so 4/5
> >> needs to be be squashed into this commit, otherwise the new fields below
> >> remain empty for it.
> > Yes, indeed, otherwise it will break bisection.
> > I was looking for a way to separate the changes for hw/ppc/spapr.c
> > into a separate patch.
> > 
> > Maybe I will leave both assignments (of the qemu_machine pointer and the fields)
> > in this patch and remove it on 5/5 (together with qemu_machine field from 
> > boards.h), leaving the 4/5 as it is?
> 
> The trouble with 2/5 is that in generic code you start using mc->
> fields, which do not get assigned by pseries. So the "mc->... = ...;"
> parts of 4/5 need to go into 2/5, whether you leave qemu_machine
> assigned or not.
> 
> Of course I understand your desire to not put everything into one huge
> commit. Maybe reordering 3/5 before this patch using ->qemu_machine
> helps? Next prepare QEMUMachineInitArgs for MachineClass (5/5). Then in
> this step you can drop qemu_machine field as suggested and any
> ->qemu_machine gets dropped in one big but mechanical change.
Thanks Andreas,
I'll try to follow this path.

Marcel
> 
> Regards,
> Andreas
>
diff mbox

Patch

diff --git a/device-hotplug.c b/device-hotplug.c
index ebfa6b1..eecb08e 100644
--- a/device-hotplug.c
+++ b/device-hotplug.c
@@ -40,7 +40,7 @@  DriveInfo *add_init_drive(const char *optstr)
         return NULL;
 
     mc = MACHINE_GET_CLASS(current_machine);
-    dinfo = drive_init(opts, mc->qemu_machine->block_default_type);
+    dinfo = drive_init(opts, mc->block_default_type);
     if (!dinfo) {
         qemu_opts_del(opts);
         return NULL;
diff --git a/qmp.c b/qmp.c
index 87a28f7..26eb589 100644
--- a/qmp.c
+++ b/qmp.c
@@ -117,8 +117,8 @@  void qmp_cpu_add(int64_t id, Error **errp)
     MachineClass *mc;
 
     mc = MACHINE_GET_CLASS(current_machine);
-    if (mc->qemu_machine->hot_add_cpu) {
-        mc->qemu_machine->hot_add_cpu(id, errp);
+    if (mc->hot_add_cpu) {
+        mc->hot_add_cpu(id, errp);
     } else {
         error_setg(errp, "Not supported");
     }
diff --git a/vl.c b/vl.c
index 9975e5a..96155ca 100644
--- a/vl.c
+++ b/vl.c
@@ -1583,8 +1583,29 @@  MachineState *current_machine;
 static void machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
-
-    mc->qemu_machine = data;
+    QEMUMachine *qm = data;
+
+    mc->name = qm->name;
+    mc->alias = qm->alias;
+    mc->desc = qm->desc;
+    mc->init = qm->init;
+    mc->reset = qm->reset;
+    mc->hot_add_cpu = qm->hot_add_cpu;
+    mc->kvm_type = qm->kvm_type;
+    mc->block_default_type = qm->block_default_type;
+    mc->max_cpus = qm->max_cpus;
+    mc->no_serial = qm->no_serial;
+    mc->no_parallel = qm->no_parallel;
+    mc->use_virtcon = qm->use_virtcon;
+    mc->use_sclp = qm->use_sclp;
+    mc->no_floppy = qm->no_floppy;
+    mc->no_cdrom = qm->no_cdrom;
+    mc->no_sdcard = qm->no_sdcard;
+    mc->is_default = qm->is_default;
+    mc->default_machine_opts = qm->default_machine_opts;
+    mc->default_boot_order = qm->default_boot_order;
+    mc->compat_props = qm->compat_props;
+    mc->hw_version = qm->hw_version;
 }
 
 int qemu_register_machine(QEMUMachine *m)
@@ -1611,12 +1632,12 @@  static MachineClass *find_machine(const char *name)
     for (el = machines; el; el = el->next) {
         MachineClass *temp = el->data;
 
-        if (!strcmp(temp->qemu_machine->name, name)) {
+        if (!strcmp(temp->name, name)) {
             mc = temp;
             break;
         }
-        if (temp->qemu_machine->alias &&
-            !strcmp(temp->qemu_machine->alias, name)) {
+        if (temp->alias &&
+            !strcmp(temp->alias, name)) {
             mc = temp;
             break;
         }
@@ -1634,7 +1655,7 @@  MachineClass *find_default_machine(void)
     for (el = machines; el; el = el->next) {
         MachineClass *temp = el->data;
 
-        if (temp->qemu_machine->is_default) {
+        if (temp->is_default) {
             mc = temp;
             break;
         }
@@ -1648,27 +1669,25 @@  MachineInfoList *qmp_query_machines(Error **errp)
 {
     GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
     MachineInfoList *mach_list = NULL;
-    QEMUMachine *m;
 
     for (el = machines; el; el = el->next) {
         MachineClass *mc = el->data;
         MachineInfoList *entry;
         MachineInfo *info;
 
-        m = mc->qemu_machine;
         info = g_malloc0(sizeof(*info));
-        if (m->is_default) {
+        if (mc->is_default) {
             info->has_is_default = true;
             info->is_default = true;
         }
 
-        if (m->alias) {
+        if (mc->alias) {
             info->has_alias = true;
-            info->alias = g_strdup(m->alias);
+            info->alias = g_strdup(mc->alias);
         }
 
-        info->name = g_strdup(m->name);
-        info->cpu_max = !m->max_cpus ? 1 : m->max_cpus;
+        info->name = g_strdup(mc->name);
+        info->cpu_max = !mc->max_cpus ? 1 : mc->max_cpus;
 
         entry = g_malloc0(sizeof(*entry));
         entry->value = info;
@@ -1874,8 +1893,8 @@  void qemu_system_reset(bool report)
 
     mc = current_machine ? MACHINE_GET_CLASS(current_machine) : NULL;
 
-    if (mc && mc->qemu_machine->reset) {
-        mc->qemu_machine->reset();
+    if (mc && mc->reset) {
+        mc->reset();
     } else {
         qemu_devices_reset();
     }
@@ -2684,12 +2703,11 @@  static MachineClass *machine_parse(const char *name)
         printf("Supported machines are:\n");
         for (el = machines; el; el = el->next) {
             MachineClass *mc = el->data;
-            QEMUMachine *m = mc->qemu_machine;
-            if (m->alias) {
-                printf("%-20s %s (alias of %s)\n", m->alias, m->desc, m->name);
+            if (mc->alias) {
+                printf("%-20s %s (alias of %s)\n", mc->alias, mc->desc, mc->name);
             }
-            printf("%-20s %s%s\n", m->name, m->desc,
-                   m->is_default ? " (default)" : "");
+            printf("%-20s %s%s\n", mc->name, mc->desc,
+                   mc->is_default ? " (default)" : "");
         }
     }
 
@@ -2943,7 +2961,7 @@  int main(int argc, char **argv, char **envp)
     const char *optarg;
     const char *loadvm = NULL;
     MachineClass *machine_class;
-    QEMUMachine *machine;
+    QEMUMachine *machine = NULL;
     const char *cpu_model;
     const char *vga_model = NULL;
     const char *qtest_chrdev = NULL;
@@ -3939,9 +3957,8 @@  int main(int argc, char **argv, char **envp)
     object_property_add_child(object_get_root(), "machine",
                               OBJECT(current_machine), &error_abort);
 
-    machine = machine_class->qemu_machine;
-    if (machine->hw_version) {
-        qemu_set_version(machine->hw_version);
+    if (machine_class->hw_version) {
+        qemu_set_version(machine_class->hw_version);
     }
 
     if (qemu_opts_foreach(qemu_find_opts("object"),
@@ -4001,11 +4018,11 @@  int main(int argc, char **argv, char **envp)
 
     smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL));
 
-    machine->max_cpus = machine->max_cpus ?: 1; /* Default to UP */
-    if (smp_cpus > machine->max_cpus) {
+    machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */
+    if (smp_cpus > machine_class->max_cpus) {
         fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus "
-                "supported by machine `%s' (%d)\n", smp_cpus,  machine->name,
-                machine->max_cpus);
+                "supported by machine `%s' (%d)\n", smp_cpus,
+                machine_class->name, machine_class->max_cpus);
         exit(1);
     }
 
@@ -4013,9 +4030,9 @@  int main(int argc, char **argv, char **envp)
      * Get the default machine options from the machine if it is not already
      * specified either by the configuration file or by the command line.
      */
-    if (machine->default_machine_opts) {
+    if (machine_class->default_machine_opts) {
         qemu_opts_set_defaults(qemu_find_opts("machine"),
-                               machine->default_machine_opts, 0);
+                               machine_class->default_machine_opts, 0);
     }
 
     qemu_opts_foreach(qemu_find_opts("device"), default_driver_check, NULL, 0);
@@ -4024,25 +4041,25 @@  int main(int argc, char **argv, char **envp)
     if (!vga_model && !default_vga) {
         vga_interface_type = VGA_DEVICE;
     }
-    if (!has_defaults || machine->no_serial) {
+    if (!has_defaults || machine_class->no_serial) {
         default_serial = 0;
     }
-    if (!has_defaults || machine->no_parallel) {
+    if (!has_defaults || machine_class->no_parallel) {
         default_parallel = 0;
     }
-    if (!has_defaults || !machine->use_virtcon) {
+    if (!has_defaults || !machine_class->use_virtcon) {
         default_virtcon = 0;
     }
-    if (!has_defaults || !machine->use_sclp) {
+    if (!has_defaults || !machine_class->use_sclp) {
         default_sclp = 0;
     }
-    if (!has_defaults || machine->no_floppy) {
+    if (!has_defaults || machine_class->no_floppy) {
         default_floppy = 0;
     }
-    if (!has_defaults || machine->no_cdrom) {
+    if (!has_defaults || machine_class->no_cdrom) {
         default_cdrom = 0;
     }
-    if (!has_defaults || machine->no_sdcard) {
+    if (!has_defaults || machine_class->no_sdcard) {
         default_sdcard = 0;
     }
     if (!has_defaults) {
@@ -4182,7 +4199,7 @@  int main(int argc, char **argv, char **envp)
     kernel_cmdline = qemu_opt_get(machine_opts, "append");
     bios_name = qemu_opt_get(machine_opts, "firmware");
 
-    boot_order = machine->default_boot_order;
+    boot_order = machine_class->default_boot_order;
     opts = qemu_opts_find(qemu_find_opts("boot-opts"), NULL);
     if (opts) {
         char *normal_boot_order;
@@ -4276,11 +4293,11 @@  int main(int argc, char **argv, char **envp)
     if (snapshot)
         qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot, NULL, 0);
     if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
-                          &machine->block_default_type, 1) != 0) {
+                          &machine_class->block_default_type, 1) != 0) {
         exit(1);
     }
 
-    default_drive(default_cdrom, snapshot, machine->block_default_type, 2,
+    default_drive(default_cdrom, snapshot, machine_class->block_default_type, 2,
                   CDROM_OPTS);
     default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
     default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
@@ -4364,8 +4381,8 @@  int main(int argc, char **argv, char **envp)
             exit (i == 1 ? 1 : 0);
     }
 
-    if (machine->compat_props) {
-        qdev_prop_register_global_list(machine->compat_props);
+    if (machine_class->compat_props) {
+        qdev_prop_register_global_list(machine_class->compat_props);
     }
     qemu_add_globals();
 
@@ -4380,7 +4397,7 @@  int main(int argc, char **argv, char **envp)
                                  .cpu_model = cpu_model };
 
     current_machine->init_args = args;
-    machine->init(&current_machine->init_args);
+    machine_class->init(&current_machine->init_args);
 
     audio_init();