[v3,1/2] vl.c: refactor current_machine as non-global variable

Commit Message

Like Xu April 15, 2019, 7:59 a.m. UTC

This patch makes the remaining dozen or so uses of the global
current_machine outside vl.c use qdev_get_machine() instead,
and then make current_machine local to vl.c instead of global.

Suggested-by: Peter Maydell <[e-mail]>
Signed-off-by: Like Xu <[e-mail]>
---
 accel/kvm/kvm-all.c | 6 ++++--
 device-hotplug.c    | 3 ++-
 device_tree.c       | 3 ++-
 exec.c              | 6 ++++--
 hw/ppc/spapr_rtas.c | 3 ++-
 include/hw/boards.h | 1 -
 migration/savevm.c  | 9 ++++++---
 qmp.c               | 3 ++-
 target/i386/kvm.c   | 3 ++-
 target/ppc/kvm.c    | 3 ++-
 vl.c                | 4 ++--
 11 files changed, 28 insertions(+), 16 deletions(-)

Comments

Eduardo Habkost April 16, 2019, 9:16 p.m. UTC | #1

On Mon, Apr 15, 2019 at 03:59:44PM +0800, Like Xu wrote:
> This patch makes the remaining dozen or so uses of the global
> current_machine outside vl.c use qdev_get_machine() instead,
> and then make current_machine local to vl.c instead of global.
> 
> Suggested-by: Peter Maydell <[e-mail]>
> Signed-off-by: Like Xu <[e-mail]>

Reviewed-by: Eduardo Habkost <[e-mail]>

Markus Armbruster April 17, 2019, 5:26 a.m. UTC | #2

Like Xu <[e-mail]> writes:

> This patch makes the remaining dozen or so uses of the global
> current_machine outside vl.c use qdev_get_machine() instead,
> and then make current_machine local to vl.c instead of global.
>
> Suggested-by: Peter Maydell <[e-mail]>
> Signed-off-by: Like Xu <[e-mail]>

I'm afraid I dislike this one, too.

The patch does not reduce global state, it's merely MICAHI (make it
complicated and hide it).

It does not improve safety, it merely turns dereferences of null
current_machine into unwanted creation of "/machine" as container (ugh),
which the next patch then fixes up to assertion failure.

The only benefit I can see is you can't assign to current_machine
outside vl.c anymore.  Nobody ever did, thus complete non-issue.

If you want to hide global state without actually reducing it, create an
accessor function.  You can then use that to replace qdev_get_machine(),
getting rid of its surprising side effect.  *That* would be an
improvement I could get behind.

Better that *hiding* use of global state would be *eliminating* use of
global state: pass current_machine around.  This isn't always practical.
But where it is, the dependence on "machine created" becomes obvious in
the code.

Eduardo Habkost April 17, 2019, 5:05 p.m. UTC | #3

On Wed, Apr 17, 2019 at 07:26:14AM +0200, Markus Armbruster wrote:
> Like Xu <[e-mail]> writes:
> 
> > This patch makes the remaining dozen or so uses of the global
> > current_machine outside vl.c use qdev_get_machine() instead,
> > and then make current_machine local to vl.c instead of global.
> >
> > Suggested-by: Peter Maydell <[e-mail]>
> > Signed-off-by: Like Xu <[e-mail]>
> 
> I'm afraid I dislike this one, too.
> 
> The patch does not reduce global state, it's merely MICAHI (make it
> complicated and hide it).
> 
> It does not improve safety, it merely turns dereferences of null
> current_machine into unwanted creation of "/machine" as container (ugh),
> which the next patch then fixes up to assertion failure.
> 
> The only benefit I can see is you can't assign to current_machine
> outside vl.c anymore.  Nobody ever did, thus complete non-issue.

The benefit I see is that we now have a single way to access the
existing global state instead of two.

> 
> If you want to hide global state without actually reducing it, create an
> accessor function.  You can then use that to replace qdev_get_machine(),
> getting rid of its surprising side effect.  *That* would be an
> improvement I could get behind.
> 
> Better that *hiding* use of global state would be *eliminating* use of
> global state: pass current_machine around.  This isn't always practical.
> But where it is, the dependence on "machine created" becomes obvious in
> the code.

I agree qdev_get_machine() has many issues.  I dislike
qdev_get_machine() a lot, and I think I had suggested we stop
using it and use current_machine instead.

But at least now we have one imperfect API instead of two
imperfect APIs.  I do think this makes future clean up work
easier.

Patch

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 241db49..d103de2 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -140,7 +140,8 @@  static const KVMCapabilityInfo kvm_required_capabilites[] = {
 
 int kvm_get_max_memslots(void)
 {
-    KVMState *s = KVM_STATE(current_machine->accelerator);
+    MachineState *ms = MACHINE(qdev_get_machine());
+    KVMState *s = KVM_STATE(ms->accelerator);
 
     return s->nr_slots;
 }
@@ -1519,7 +1520,8 @@  static int kvm_max_vcpu_id(KVMState *s)
 
 bool kvm_vcpu_id_is_valid(int vcpu_id)
 {
-    KVMState *s = KVM_STATE(current_machine->accelerator);
+    MachineState *ms = MACHINE(qdev_get_machine());
+    KVMState *s = KVM_STATE(ms->accelerator);
     return vcpu_id >= 0 && vcpu_id < kvm_max_vcpu_id(s);
 }
 
diff --git a/device-hotplug.c b/device-hotplug.c
index 6153259..d31c1f8 100644
--- a/device-hotplug.c
+++ b/device-hotplug.c
@@ -37,6 +37,7 @@ 
 
 static DriveInfo *add_init_drive(const char *optstr)
 {
+    MachineState *ms = MACHINE(qdev_get_machine());
     Error *err = NULL;
     DriveInfo *dinfo;
     QemuOpts *opts;
@@ -46,7 +47,7 @@  static DriveInfo *add_init_drive(const char *optstr)
     if (!opts)
         return NULL;
 
-    mc = MACHINE_GET_CLASS(current_machine);
+    mc = MACHINE_GET_CLASS(ms);
     dinfo = drive_new(opts, mc->block_default_type, &err);
     if (err) {
         error_report_err(err);
diff --git a/device_tree.c b/device_tree.c
index f8b46b3..3294ef6 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -459,6 +459,7 @@  int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
 
 uint32_t qemu_fdt_alloc_phandle(void *fdt)
 {
+    MachineState *ms = MACHINE(qdev_get_machine());
     static int phandle = 0x0;
 
     /*
@@ -466,7 +467,7 @@  uint32_t qemu_fdt_alloc_phandle(void *fdt)
      * which phandle id to start allocating phandles.
      */
     if (!phandle) {
-        phandle = machine_phandle_start(current_machine);
+        phandle = machine_phandle_start(ms);
     }
 
     if (!phandle) {
diff --git a/exec.c b/exec.c
index 6ab62f4..15ff2b1 100644
--- a/exec.c
+++ b/exec.c
@@ -1969,10 +1969,11 @@  static unsigned long last_ram_page(void)
 
 static void qemu_ram_setup_dump(void *addr, ram_addr_t size)
 {
+    MachineState *ms = MACHINE(qdev_get_machine());
     int ret;
 
     /* Use MADV_DONTDUMP, if user doesn't want the guest memory in the core */
-    if (!machine_dump_guest_core(current_machine)) {
+    if (!machine_dump_guest_core(ms)) {
         ret = qemu_madvise(addr, size, QEMU_MADV_DONTDUMP);
         if (ret) {
             perror("qemu_madvise");
@@ -2094,7 +2095,8 @@  size_t qemu_ram_pagesize_largest(void)
 
 static int memory_try_enable_merging(void *addr, size_t len)
 {
-    if (!machine_mem_merge(current_machine)) {
+    MachineState *ms = MACHINE(qdev_get_machine());
+    if (!machine_mem_merge(ms)) {
         /* disabled by the user */
         return 0;
     }
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 24c45b1..51e320d 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -231,6 +231,7 @@  static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
                                           target_ulong args,
                                           uint32_t nret, target_ulong rets)
 {
+    MachineState *ms = MACHINE(spapr);
     target_ulong parameter = rtas_ld(args, 0);
     target_ulong buffer = rtas_ld(args, 1);
     target_ulong length = rtas_ld(args, 2);
@@ -243,7 +244,7 @@  static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
                                           "DesProcs=%d,"
                                           "MaxPlatProcs=%d",
                                           max_cpus,
-                                          current_machine->ram_size / MiB,
+                                          ms->ram_size / MiB,
                                           smp_cpus,
                                           max_cpus);
         ret = sysparm_st(buffer, length, param_val, strlen(param_val) + 1);
diff --git a/include/hw/boards.h b/include/hw/boards.h
index e231860..1d598c8 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -58,7 +58,6 @@  void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
     OBJECT_CLASS_CHECK(MachineClass, (klass), TYPE_MACHINE)
 
 MachineClass *find_default_machine(void);
-extern MachineState *current_machine;
 
 void machine_run_board_init(MachineState *machine);
 bool machine_usb(MachineState *machine);
diff --git a/migration/savevm.c b/migration/savevm.c
index 34bcad3..4261061 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -286,8 +286,9 @@  static uint32_t get_validatable_capabilities_count(void)
 
 static int configuration_pre_save(void *opaque)
 {
+    MachineState *ms = MACHINE(qdev_get_machine());
     SaveState *state = opaque;
-    const char *current_name = MACHINE_GET_CLASS(current_machine)->name;
+    const char *current_name = MACHINE_GET_CLASS(ms)->name;
     MigrationState *s = migrate_get_current();
     int i, j;
 
@@ -355,8 +356,9 @@  static bool configuration_validate_capabilities(SaveState *state)
 
 static int configuration_post_load(void *opaque, int version_id)
 {
+    MachineState *ms = MACHINE(qdev_get_machine());
     SaveState *state = opaque;
-    const char *current_name = MACHINE_GET_CLASS(current_machine)->name;
+    const char *current_name = MACHINE_GET_CLASS(ms)->name;
 
     if (strncmp(state->name, current_name, state->len) != 0) {
         error_report("Machine type received is '%.*s' and local is '%s'",
@@ -566,9 +568,10 @@  static void dump_vmstate_vmsd(FILE *out_file,
 
 static void dump_machine_type(FILE *out_file)
 {
+    MachineState *ms = MACHINE(qdev_get_machine());
     MachineClass *mc;
 
-    mc = MACHINE_GET_CLASS(current_machine);
+    mc = MACHINE_GET_CLASS(ms);
 
     fprintf(out_file, "  \"vmschkmachine\": {\n");
     fprintf(out_file, "    \"Name\": \"%s\"\n", mc->name);
diff --git a/qmp.c b/qmp.c
index b92d62c..f2a5473 100644
--- a/qmp.c
+++ b/qmp.c
@@ -119,9 +119,10 @@  void qmp_system_powerdown(Error **erp)
 
 void qmp_cpu_add(int64_t id, Error **errp)
 {
+    MachineState *ms = MACHINE(qdev_get_machine());
     MachineClass *mc;
 
-    mc = MACHINE_GET_CLASS(current_machine);
+    mc = MACHINE_GET_CLASS(ms);
     if (mc->hot_add_cpu) {
         mc->hot_add_cpu(id, errp);
     } else {
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 3b29ce5..b25d766 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -134,7 +134,8 @@  bool kvm_allows_irq0_override(void)
 
 static bool kvm_x2apic_api_set_flags(uint64_t flags)
 {
-    KVMState *s = KVM_STATE(current_machine->accelerator);
+    MachineState *ms = MACHINE(qdev_get_machine());
+    KVMState *s = KVM_STATE(ms->accelerator);
 
     return !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags);
 }
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 2427c8e..c3bea37 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -276,7 +276,8 @@  static void kvm_get_smmu_info(struct kvm_ppc_smmu_info *info, Error **errp)
 
 struct ppc_radix_page_info *kvm_get_radix_page_info(void)
 {
-    KVMState *s = KVM_STATE(current_machine->accelerator);
+    MachineState *ms = MACHINE(qdev_get_machine());
+    KVMState *s = KVM_STATE(ms->accelerator);
     struct ppc_radix_page_info *radix_page_info;
     struct kvm_ppc_rmmu_info rmmu_info;
     int i;
diff --git a/vl.c b/vl.c
index c696ad2..a1a8b24 100644
--- a/vl.c
+++ b/vl.c
@@ -1266,6 +1266,8 @@  static QemuOptsList qemu_smp_opts = {
     },
 };
 
+static MachineState *current_machine;
+
 static void smp_parse(QemuOpts *opts)
 {
     if (opts) {
@@ -1463,8 +1465,6 @@  static int usb_parse(const char *cmdline)
 /***********************************************************/
 /* machine registration */
 
-MachineState *current_machine;
-
 static MachineClass *find_machine(const char *name)
 {
     GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);