[Qemu-devel] [PATCH v3 0/2] vl.c: make current_machine as non-global variable

[Qemu-devel] [PATCH v3 0/2] vl.c: make current_machine as non-global variable

[Like Xu]

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.

With type assertion in qdev_get_machine(), it will be hard to
misuse this function if machine hasn't been created yet.
For obj-common cases, qdev_get_machine_uncheck() is applied
without semantic change.

---
Changes in v3:
    - add TYPE_MACHINE assertion for qdev_get_machine() usage
    - apply qdev_get_machine_uncheck() for obj-common usage
Changes in v2:
    - make the variable current_machine "static" (Thomas Huth)

Like Xu (2):
  vl.c: refactor current_machine as non-global variable
  core/qdev: refactor qdev_get_machine() with type assertion

 accel/kvm/kvm-all.c    |  6 ++++--
 device-hotplug.c       |  3 ++-
 device_tree.c          |  3 ++-
 exec.c                 |  6 ++++--
 hw/core/qdev.c         | 16 +++++++++++++---
 hw/ppc/spapr_rtas.c    |  3 ++-
 include/hw/boards.h    |  1 -
 include/hw/qdev-core.h |  1 +
 migration/savevm.c     |  9 ++++++---
 qmp.c                  |  3 ++-
 qom/cpu.c              |  5 +++--
 target/i386/kvm.c      |  3 ++-
 target/ppc/kvm.c       |  3 ++-
 vl.c                   |  4 ++--
 14 files changed, 45 insertions(+), 21 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 0/2] vl.c: make current_machine as non-global variable

[Like Xu]

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.

With type assertion in qdev_get_machine(), it will be hard to
misuse this function if machine hasn't been created yet.
For obj-common cases, qdev_get_machine_uncheck() is applied
without semantic change.

---
Changes in v3:
    - add TYPE_MACHINE assertion for qdev_get_machine() usage
    - apply qdev_get_machine_uncheck() for obj-common usage
Changes in v2:
    - make the variable current_machine "static" (Thomas Huth)

Like Xu (2):
  vl.c: refactor current_machine as non-global variable
  core/qdev: refactor qdev_get_machine() with type assertion

 accel/kvm/kvm-all.c    |  6 ++++--
 device-hotplug.c       |  3 ++-
 device_tree.c          |  3 ++-
 exec.c                 |  6 ++++--
 hw/core/qdev.c         | 16 +++++++++++++---
 hw/ppc/spapr_rtas.c    |  3 ++-
 include/hw/boards.h    |  1 -
 include/hw/qdev-core.h |  1 +
 migration/savevm.c     |  9 ++++++---
 qmp.c                  |  3 ++-
 qom/cpu.c              |  5 +++--
 target/i386/kvm.c      |  3 ++-
 target/ppc/kvm.c       |  3 ++-
 vl.c                   |  4 ++--
 14 files changed, 45 insertions(+), 21 deletions(-)

-- 
1.8.3.1

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

[Like Xu]

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 <peter.maydell@linaro.org>
Signed-off-by: Like Xu <like.xu@linux.intel.com>
---
 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(-)

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

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

[Like Xu]

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 <peter.maydell@linaro.org>
Signed-off-by: Like Xu <like.xu@linux.intel.com>
---
 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(-)

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

Re: [Qemu-devel] [PATCH v3 1/2] vl.c: refactor current_machine as non-global variable

[Eduardo Habkost]

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 <peter.maydell@linaro.org>
> Signed-off-by: Like Xu <like.xu@linux.intel.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 1/2] vl.c: refactor current_machine as non-global variable

[Eduardo Habkost]

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 <peter.maydell@linaro.org>
> Signed-off-by: Like Xu <like.xu@linux.intel.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 1/2] vl.c: refactor current_machine as non-global variable

[Markus Armbruster]

Like Xu <like.xu@linux.intel.com> 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 <peter.maydell@linaro.org>
> Signed-off-by: Like Xu <like.xu@linux.intel.com>

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.

Re: [Qemu-devel] [PATCH v3 1/2] vl.c: refactor current_machine as non-global variable

[Markus Armbruster]

Like Xu <like.xu@linux.intel.com> 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 <peter.maydell@linaro.org>
> Signed-off-by: Like Xu <like.xu@linux.intel.com>

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.

Re: [Qemu-devel] [PATCH v3 1/2] vl.c: refactor current_machine as non-global variable

[Eduardo Habkost]

On Wed, Apr 17, 2019 at 07:26:14AM +0200, Markus Armbruster wrote:
> Like Xu <like.xu@linux.intel.com> 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 <peter.maydell@linaro.org>
> > Signed-off-by: Like Xu <like.xu@linux.intel.com>
> 
> 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.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 1/2] vl.c: refactor current_machine as non-global variable

[Eduardo Habkost]

On Wed, Apr 17, 2019 at 07:26:14AM +0200, Markus Armbruster wrote:
> Like Xu <like.xu@linux.intel.com> 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 <peter.maydell@linaro.org>
> > Signed-off-by: Like Xu <like.xu@linux.intel.com>
> 
> 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.

-- 
Eduardo

[Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion

[Like Xu]

To avoid the misuse of qdev_get_machine() if machine hasn't been created yet,
this patch uses qdev_get_machine_uncheck() for obj-common (share with user-only
mode) and adds type assertion to qdev_get_machine() in system-emulation mode.

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Like Xu <like.xu@linux.intel.com>
---
 hw/core/qdev.c         | 16 +++++++++++++---
 include/hw/qdev-core.h |  1 +
 qom/cpu.c              |  5 +++--
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index f9b6efe..8232216 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -223,7 +223,7 @@ HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev)
 {
     MachineState *machine;
     MachineClass *mc;
-    Object *m_obj = qdev_get_machine();
+    Object *m_obj = qdev_get_machine_uncheck();
 
     if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
         machine = MACHINE(m_obj);
@@ -815,7 +815,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
         if (!obj->parent) {
             gchar *name = g_strdup_printf("device[%d]", unattached_count++);
 
-            object_property_add_child(container_get(qdev_get_machine(),
+            object_property_add_child(container_get(qdev_get_machine_uncheck(),
                                                     "/unattached"),
                                       name, obj, &error_abort);
             unattached_parent = true;
@@ -1095,7 +1095,7 @@ void device_reset(DeviceState *dev)
     }
 }
 
-Object *qdev_get_machine(void)
+Object *qdev_get_machine_uncheck(void)
 {
     static Object *dev;
 
@@ -1106,6 +1106,16 @@ Object *qdev_get_machine(void)
     return dev;
 }
 
+Object *qdev_get_machine(void)
+{
+    static Object *dev;
+
+    dev = qdev_get_machine_uncheck();
+    assert(object_dynamic_cast(dev, TYPE_MACHINE) != NULL);
+
+    return dev;
+}
+
 static const TypeInfo device_type_info = {
     .name = TYPE_DEVICE,
     .parent = TYPE_OBJECT,
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 33ed3b8..e7c6a5a 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -429,6 +429,7 @@ const struct VMStateDescription *qdev_get_vmsd(DeviceState *dev);
 
 const char *qdev_fw_name(DeviceState *dev);
 
+Object *qdev_get_machine_uncheck(void);
 Object *qdev_get_machine(void);
 
 /* FIXME: make this a link<> */
diff --git a/qom/cpu.c b/qom/cpu.c
index a8d2958..bb877d5 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -325,9 +325,10 @@ static void cpu_common_parse_features(const char *typename, char *features,
 static void cpu_common_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cpu = CPU(dev);
-    Object *machine = qdev_get_machine();
+    Object *machine = qdev_get_machine_uncheck();
 
-    /* qdev_get_machine() can return something that's not TYPE_MACHINE
+    /*
+     * qdev_get_machine_uncheck() can return something that's not TYPE_MACHINE
      * if this is one of the user-only emulators; in that case there's
      * no need to check the ignore_memory_transaction_failures board flag.
      */
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion

[Like Xu]

To avoid the misuse of qdev_get_machine() if machine hasn't been created yet,
this patch uses qdev_get_machine_uncheck() for obj-common (share with user-only
mode) and adds type assertion to qdev_get_machine() in system-emulation mode.

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Like Xu <like.xu@linux.intel.com>
---
 hw/core/qdev.c         | 16 +++++++++++++---
 include/hw/qdev-core.h |  1 +
 qom/cpu.c              |  5 +++--
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index f9b6efe..8232216 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -223,7 +223,7 @@ HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev)
 {
     MachineState *machine;
     MachineClass *mc;
-    Object *m_obj = qdev_get_machine();
+    Object *m_obj = qdev_get_machine_uncheck();
 
     if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
         machine = MACHINE(m_obj);
@@ -815,7 +815,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
         if (!obj->parent) {
             gchar *name = g_strdup_printf("device[%d]", unattached_count++);
 
-            object_property_add_child(container_get(qdev_get_machine(),
+            object_property_add_child(container_get(qdev_get_machine_uncheck(),
                                                     "/unattached"),
                                       name, obj, &error_abort);
             unattached_parent = true;
@@ -1095,7 +1095,7 @@ void device_reset(DeviceState *dev)
     }
 }
 
-Object *qdev_get_machine(void)
+Object *qdev_get_machine_uncheck(void)
 {
     static Object *dev;
 
@@ -1106,6 +1106,16 @@ Object *qdev_get_machine(void)
     return dev;
 }
 
+Object *qdev_get_machine(void)
+{
+    static Object *dev;
+
+    dev = qdev_get_machine_uncheck();
+    assert(object_dynamic_cast(dev, TYPE_MACHINE) != NULL);
+
+    return dev;
+}
+
 static const TypeInfo device_type_info = {
     .name = TYPE_DEVICE,
     .parent = TYPE_OBJECT,
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 33ed3b8..e7c6a5a 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -429,6 +429,7 @@ const struct VMStateDescription *qdev_get_vmsd(DeviceState *dev);
 
 const char *qdev_fw_name(DeviceState *dev);
 
+Object *qdev_get_machine_uncheck(void);
 Object *qdev_get_machine(void);
 
 /* FIXME: make this a link<> */
diff --git a/qom/cpu.c b/qom/cpu.c
index a8d2958..bb877d5 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -325,9 +325,10 @@ static void cpu_common_parse_features(const char *typename, char *features,
 static void cpu_common_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cpu = CPU(dev);
-    Object *machine = qdev_get_machine();
+    Object *machine = qdev_get_machine_uncheck();
 
-    /* qdev_get_machine() can return something that's not TYPE_MACHINE
+    /*
+     * qdev_get_machine_uncheck() can return something that's not TYPE_MACHINE
      * if this is one of the user-only emulators; in that case there's
      * no need to check the ignore_memory_transaction_failures board flag.
      */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion

[Eduardo Habkost]

On Mon, Apr 15, 2019 at 03:59:45PM +0800, Like Xu wrote:
> To avoid the misuse of qdev_get_machine() if machine hasn't been created yet,
> this patch uses qdev_get_machine_uncheck() for obj-common (share with user-only
> mode) and adds type assertion to qdev_get_machine() in system-emulation mode.
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Like Xu <like.xu@linux.intel.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

I'm queueing the series on machine-next, thanks!

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion

[Eduardo Habkost]

On Mon, Apr 15, 2019 at 03:59:45PM +0800, Like Xu wrote:
> To avoid the misuse of qdev_get_machine() if machine hasn't been created yet,
> this patch uses qdev_get_machine_uncheck() for obj-common (share with user-only
> mode) and adds type assertion to qdev_get_machine() in system-emulation mode.
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Like Xu <like.xu@linux.intel.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

I'm queueing the series on machine-next, thanks!

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Mon, Apr 15, 2019 at 03:59:45PM +0800, Like Xu wrote:
>> To avoid the misuse of qdev_get_machine() if machine hasn't been created yet,
>> this patch uses qdev_get_machine_uncheck() for obj-common (share with user-only
>> mode) and adds type assertion to qdev_get_machine() in system-emulation mode.
>> 
>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: Like Xu <like.xu@linux.intel.com>
>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>
> I'm queueing the series on machine-next, thanks!

Hold your horses, please.

I dislike the name qdev_get_machine_uncheck().  I could live with
qdev_get_machine_unchecked().

However, I doubt this is the right approach.

The issue at hand is undisciplined creation of QOM object /machine.

This patch adds an asseertion "undisciplined creation of /machine didn't
create crap", but only in some places.

I think we should never create /machine as (surprising!) side effect of
qdev_get_machine().  Create it explicitly instead, and have
qdev_get_machine() use object_resolve_path("/machine", NULL) to get it.
Look ma, no side effects.

Re: [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Mon, Apr 15, 2019 at 03:59:45PM +0800, Like Xu wrote:
>> To avoid the misuse of qdev_get_machine() if machine hasn't been created yet,
>> this patch uses qdev_get_machine_uncheck() for obj-common (share with user-only
>> mode) and adds type assertion to qdev_get_machine() in system-emulation mode.
>> 
>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: Like Xu <like.xu@linux.intel.com>
>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>
> I'm queueing the series on machine-next, thanks!

Hold your horses, please.

I dislike the name qdev_get_machine_uncheck().  I could live with
qdev_get_machine_unchecked().

However, I doubt this is the right approach.

The issue at hand is undisciplined creation of QOM object /machine.

This patch adds an asseertion "undisciplined creation of /machine didn't
create crap", but only in some places.

I think we should never create /machine as (surprising!) side effect of
qdev_get_machine().  Create it explicitly instead, and have
qdev_get_machine() use object_resolve_path("/machine", NULL) to get it.
Look ma, no side effects.

Re: [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion

[Eduardo Habkost]

On Wed, Apr 17, 2019 at 07:14:10AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Mon, Apr 15, 2019 at 03:59:45PM +0800, Like Xu wrote:
> >> To avoid the misuse of qdev_get_machine() if machine hasn't been created yet,
> >> this patch uses qdev_get_machine_uncheck() for obj-common (share with user-only
> >> mode) and adds type assertion to qdev_get_machine() in system-emulation mode.
> >> 
> >> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> >> Signed-off-by: Like Xu <like.xu@linux.intel.com>
> >
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> >
> > I'm queueing the series on machine-next, thanks!
> 
> Hold your horses, please.
> 
> I dislike the name qdev_get_machine_uncheck().  I could live with
> qdev_get_machine_unchecked().
> 
> However, I doubt this is the right approach.
> 
> The issue at hand is undisciplined creation of QOM object /machine.
> 
> This patch adds an asseertion "undisciplined creation of /machine didn't
> create crap", but only in some places.
> 
> I think we should never create /machine as (surprising!) side effect of
> qdev_get_machine().  Create it explicitly instead, and have
> qdev_get_machine() use object_resolve_path("/machine", NULL) to get it.
> Look ma, no side effects.

OK, I'm dropping this one while we discuss it.

I really miss a good explanation why qdev_get_machine_unchecked()
needs to exist.  When exactly do we want /machine to exist but
not be TYPE_MACHINE?  Why?

Once the expectations and use cases are explained, we can choose
a better name for qdev_get_machine_unchecked() and document it
properly.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion

[Eduardo Habkost]

On Wed, Apr 17, 2019 at 07:14:10AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Mon, Apr 15, 2019 at 03:59:45PM +0800, Like Xu wrote:
> >> To avoid the misuse of qdev_get_machine() if machine hasn't been created yet,
> >> this patch uses qdev_get_machine_uncheck() for obj-common (share with user-only
> >> mode) and adds type assertion to qdev_get_machine() in system-emulation mode.
> >> 
> >> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> >> Signed-off-by: Like Xu <like.xu@linux.intel.com>
> >
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> >
> > I'm queueing the series on machine-next, thanks!
> 
> Hold your horses, please.
> 
> I dislike the name qdev_get_machine_uncheck().  I could live with
> qdev_get_machine_unchecked().
> 
> However, I doubt this is the right approach.
> 
> The issue at hand is undisciplined creation of QOM object /machine.
> 
> This patch adds an asseertion "undisciplined creation of /machine didn't
> create crap", but only in some places.
> 
> I think we should never create /machine as (surprising!) side effect of
> qdev_get_machine().  Create it explicitly instead, and have
> qdev_get_machine() use object_resolve_path("/machine", NULL) to get it.
> Look ma, no side effects.

OK, I'm dropping this one while we discuss it.

I really miss a good explanation why qdev_get_machine_unchecked()
needs to exist.  When exactly do we want /machine to exist but
not be TYPE_MACHINE?  Why?

Once the expectations and use cases are explained, we can choose
a better name for qdev_get_machine_unchecked() and document it
properly.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion

[Like Xu]

On 2019/4/18 1:10, Eduardo Habkost wrote:
> On Wed, Apr 17, 2019 at 07:14:10AM +0200, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>
>>> On Mon, Apr 15, 2019 at 03:59:45PM +0800, Like Xu wrote:
>>>> To avoid the misuse of qdev_get_machine() if machine hasn't been created yet,
>>>> this patch uses qdev_get_machine_uncheck() for obj-common (share with user-only
>>>> mode) and adds type assertion to qdev_get_machine() in system-emulation mode.
>>>>
>>>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>>>> Signed-off-by: Like Xu <like.xu@linux.intel.com>
>>>
>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>>>
>>> I'm queueing the series on machine-next, thanks!
>>
>> Hold your horses, please.
>>
>> I dislike the name qdev_get_machine_uncheck().  I could live with
>> qdev_get_machine_unchecked().
>>
>> However, I doubt this is the right approach.
>>
>> The issue at hand is undisciplined creation of QOM object /machine.
>>
>> This patch adds an asseertion "undisciplined creation of /machine didn't
>> create crap", but only in some places.
>>
>> I think we should never create /machine as (surprising!) side effect of
>> qdev_get_machine().  Create it explicitly instead, and have
>> qdev_get_machine() use object_resolve_path("/machine", NULL) to get it.
>> Look ma, no side effects.
> 
> OK, I'm dropping this one while we discuss it.
> 
> I really miss a good explanation why qdev_get_machine_unchecked()
> needs to exist.  When exactly do we want /machine to exist but
> not be TYPE_MACHINE?  Why?

AFAICT, there is no such "/machine" that is not of type TYPE_MACHINE.

The original qdev_get_machine() would always return a "/container" 
object in user-only mode and there is none TYPE_MACHINE object.

In system emulation mode, it returns the same "/container" object at the 
beginning, until we initialize and add a TYPE_MACHINE object to the 
"/container" as a child and it would return OBJECT(current_machine)
for later usages.

The starting point is to avoid using the legacy qdev_get_machine()
in system emulation mode when we haven't added the "/machine" object.
As a result, we introduced type checking assertions to avoid premature 
invocations.

In this proposal, the qdev_get_machine_unchecked() is only used
in user-only mode, part of which shares with system emulation mode
(such as device_set_realized, cpu_common_realizefn). The new 
qdev_get_machine() is only used in system emulation mode and type 
checking assertion does reduce the irrational use of this function (if 
any in the future).

We all agree to use this qdev_get_machine() as little as possible
and this patch could make future clean up work easier.

> 
> Once the expectations and use cases are explained, we can choose
> a better name for qdev_get_machine_unchecked() and document it
> properly.
> 

Re: [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion

[Eduardo Habkost]

On Tue, Apr 23, 2019 at 03:59:31PM +0800, Like Xu wrote:
> On 2019/4/18 1:10, Eduardo Habkost wrote:
> > On Wed, Apr 17, 2019 at 07:14:10AM +0200, Markus Armbruster wrote:
> > > Eduardo Habkost <ehabkost@redhat.com> writes:
> > > 
> > > > On Mon, Apr 15, 2019 at 03:59:45PM +0800, Like Xu wrote:
> > > > > To avoid the misuse of qdev_get_machine() if machine hasn't been created yet,
> > > > > this patch uses qdev_get_machine_uncheck() for obj-common (share with user-only
> > > > > mode) and adds type assertion to qdev_get_machine() in system-emulation mode.
> > > > > 
> > > > > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > > > > Signed-off-by: Like Xu <like.xu@linux.intel.com>
> > > > 
> > > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > 
> > > > I'm queueing the series on machine-next, thanks!
> > > 
> > > Hold your horses, please.
> > > 
> > > I dislike the name qdev_get_machine_uncheck().  I could live with
> > > qdev_get_machine_unchecked().
> > > 
> > > However, I doubt this is the right approach.
> > > 
> > > The issue at hand is undisciplined creation of QOM object /machine.
> > > 
> > > This patch adds an asseertion "undisciplined creation of /machine didn't
> > > create crap", but only in some places.
> > > 
> > > I think we should never create /machine as (surprising!) side effect of
> > > qdev_get_machine().  Create it explicitly instead, and have
> > > qdev_get_machine() use object_resolve_path("/machine", NULL) to get it.
> > > Look ma, no side effects.
> > 
> > OK, I'm dropping this one while we discuss it.
> > 
> > I really miss a good explanation why qdev_get_machine_unchecked()
> > needs to exist.  When exactly do we want /machine to exist but
> > not be TYPE_MACHINE?  Why?
> 
> AFAICT, there is no such "/machine" that is not of type TYPE_MACHINE.
> 
> The original qdev_get_machine() would always return a "/container" object in
> user-only mode and there is none TYPE_MACHINE object.

I'm confused.  Both qdev_get_machine() and
qdev_get_machine_unchecked() still return the object at
"/machine".  On softmmu, /machine will be of type TYPE_MACHINE.
On user-only, /machine will be of type "container".


> 
> In system emulation mode, it returns the same "/container" object at the
> beginning, until we initialize and add a TYPE_MACHINE object to the
> "/container" as a child and it would return OBJECT(current_machine)
> for later usages.
> 
> The starting point is to avoid using the legacy qdev_get_machine()
> in system emulation mode when we haven't added the "/machine" object.
> As a result, we introduced type checking assertions to avoid premature
> invocations.

I believe Markus is suggesting that avoiding unwanted side
effects is even better than doing type checking after it's
already too late.  In other words, it doesn't make sense to call
container_get("/machine") on system emulation mode.


> 
> In this proposal, the qdev_get_machine_unchecked() is only used
> in user-only mode, part of which shares with system emulation mode
> (such as device_set_realized, cpu_common_realizefn). The new
> qdev_get_machine() is only used in system emulation mode and type checking
> assertion does reduce the irrational use of this function (if any in the
> future).

This part confuses me as well.  qdev_get_machine_unchecked() is
used in both user-only and softmmu, isn't?  Thus we can't say it
is only used in user-only mode.

I think we all agree that qdev_get_machine() should eventually be
available in softmmu only.

But I don't think we agree when it would be appropriate to call
qdev_get_machine_unchecked() instead of qdev_get_machine().

On both examples in your patch, the code checks for TYPE_MACHINE
immediately after calling qdev_get_machine_unchecked().  If that
code is only useful in softmmu mode, when would anybody want to
call qdev_get_machine_unchecked() in user-only mode?


> 
> We all agree to use this qdev_get_machine() as little as possible
> and this patch could make future clean up work easier.
> 
> > 
> > Once the expectations and use cases are explained, we can choose
> > a better name for qdev_get_machine_unchecked() and document it
> > properly.
> > 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion

[Eduardo Habkost]

On Tue, Apr 23, 2019 at 03:59:31PM +0800, Like Xu wrote:
> On 2019/4/18 1:10, Eduardo Habkost wrote:
> > On Wed, Apr 17, 2019 at 07:14:10AM +0200, Markus Armbruster wrote:
> > > Eduardo Habkost <ehabkost@redhat.com> writes:
> > > 
> > > > On Mon, Apr 15, 2019 at 03:59:45PM +0800, Like Xu wrote:
> > > > > To avoid the misuse of qdev_get_machine() if machine hasn't been created yet,
> > > > > this patch uses qdev_get_machine_uncheck() for obj-common (share with user-only
> > > > > mode) and adds type assertion to qdev_get_machine() in system-emulation mode.
> > > > > 
> > > > > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > > > > Signed-off-by: Like Xu <like.xu@linux.intel.com>
> > > > 
> > > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > 
> > > > I'm queueing the series on machine-next, thanks!
> > > 
> > > Hold your horses, please.
> > > 
> > > I dislike the name qdev_get_machine_uncheck().  I could live with
> > > qdev_get_machine_unchecked().
> > > 
> > > However, I doubt this is the right approach.
> > > 
> > > The issue at hand is undisciplined creation of QOM object /machine.
> > > 
> > > This patch adds an asseertion "undisciplined creation of /machine didn't
> > > create crap", but only in some places.
> > > 
> > > I think we should never create /machine as (surprising!) side effect of
> > > qdev_get_machine().  Create it explicitly instead, and have
> > > qdev_get_machine() use object_resolve_path("/machine", NULL) to get it.
> > > Look ma, no side effects.
> > 
> > OK, I'm dropping this one while we discuss it.
> > 
> > I really miss a good explanation why qdev_get_machine_unchecked()
> > needs to exist.  When exactly do we want /machine to exist but
> > not be TYPE_MACHINE?  Why?
> 
> AFAICT, there is no such "/machine" that is not of type TYPE_MACHINE.
> 
> The original qdev_get_machine() would always return a "/container" object in
> user-only mode and there is none TYPE_MACHINE object.

I'm confused.  Both qdev_get_machine() and
qdev_get_machine_unchecked() still return the object at
"/machine".  On softmmu, /machine will be of type TYPE_MACHINE.
On user-only, /machine will be of type "container".


> 
> In system emulation mode, it returns the same "/container" object at the
> beginning, until we initialize and add a TYPE_MACHINE object to the
> "/container" as a child and it would return OBJECT(current_machine)
> for later usages.
> 
> The starting point is to avoid using the legacy qdev_get_machine()
> in system emulation mode when we haven't added the "/machine" object.
> As a result, we introduced type checking assertions to avoid premature
> invocations.

I believe Markus is suggesting that avoiding unwanted side
effects is even better than doing type checking after it's
already too late.  In other words, it doesn't make sense to call
container_get("/machine") on system emulation mode.


> 
> In this proposal, the qdev_get_machine_unchecked() is only used
> in user-only mode, part of which shares with system emulation mode
> (such as device_set_realized, cpu_common_realizefn). The new
> qdev_get_machine() is only used in system emulation mode and type checking
> assertion does reduce the irrational use of this function (if any in the
> future).

This part confuses me as well.  qdev_get_machine_unchecked() is
used in both user-only and softmmu, isn't?  Thus we can't say it
is only used in user-only mode.

I think we all agree that qdev_get_machine() should eventually be
available in softmmu only.

But I don't think we agree when it would be appropriate to call
qdev_get_machine_unchecked() instead of qdev_get_machine().

On both examples in your patch, the code checks for TYPE_MACHINE
immediately after calling qdev_get_machine_unchecked().  If that
code is only useful in softmmu mode, when would anybody want to
call qdev_get_machine_unchecked() in user-only mode?


> 
> We all agree to use this qdev_get_machine() as little as possible
> and this patch could make future clean up work easier.
> 
> > 
> > Once the expectations and use cases are explained, we can choose
> > a better name for qdev_get_machine_unchecked() and document it
> > properly.
> > 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion

[Like Xu]

On 2019/4/25 1:21, Eduardo Habkost wrote:
> On Tue, Apr 23, 2019 at 03:59:31PM +0800, Like Xu wrote:
>> On 2019/4/18 1:10, Eduardo Habkost wrote:
>>> On Wed, Apr 17, 2019 at 07:14:10AM +0200, Markus Armbruster wrote:
>>>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>>>
>>>>> On Mon, Apr 15, 2019 at 03:59:45PM +0800, Like Xu wrote:
>>>>>> To avoid the misuse of qdev_get_machine() if machine hasn't been created yet,
>>>>>> this patch uses qdev_get_machine_uncheck() for obj-common (share with user-only
>>>>>> mode) and adds type assertion to qdev_get_machine() in system-emulation mode.
>>>>>>
>>>>>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>>>>>> Signed-off-by: Like Xu <like.xu@linux.intel.com>
>>>>>
>>>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>>>>>
>>>>> I'm queueing the series on machine-next, thanks!
>>>>
>>>> Hold your horses, please.
>>>>
>>>> I dislike the name qdev_get_machine_uncheck().  I could live with
>>>> qdev_get_machine_unchecked().
>>>>
>>>> However, I doubt this is the right approach.
>>>>
>>>> The issue at hand is undisciplined creation of QOM object /machine.
>>>>
>>>> This patch adds an asseertion "undisciplined creation of /machine didn't
>>>> create crap", but only in some places.
>>>>
>>>> I think we should never create /machine as (surprising!) side effect of
>>>> qdev_get_machine().  Create it explicitly instead, and have
>>>> qdev_get_machine() use object_resolve_path("/machine", NULL) to get it.
>>>> Look ma, no side effects.
>>>
>>> OK, I'm dropping this one while we discuss it.
>>>
>>> I really miss a good explanation why qdev_get_machine_unchecked()
>>> needs to exist.  When exactly do we want /machine to exist but
>>> not be TYPE_MACHINE?  Why?
>>
>> AFAICT, there is no such "/machine" that is not of type TYPE_MACHINE.
>>
>> The original qdev_get_machine() would always return a "/container" object in
>> user-only mode and there is none TYPE_MACHINE object.
> 
> I'm confused.  Both qdev_get_machine() and
> qdev_get_machine_unchecked() still return the object at
> "/machine".  On softmmu, /machine will be of type TYPE_MACHINE.
> On user-only, /machine will be of type "container".
> 
> 
>>
>> In system emulation mode, it returns the same "/container" object at the
>> beginning, until we initialize and add a TYPE_MACHINE object to the
>> "/container" as a child and it would return OBJECT(current_machine)
>> for later usages.
>>
>> The starting point is to avoid using the legacy qdev_get_machine()
>> in system emulation mode when we haven't added the "/machine" object.
>> As a result, we introduced type checking assertions to avoid premature
>> invocations.
> 
> I believe Markus is suggesting that avoiding unwanted side
> effects is even better than doing type checking after it's
> already too late.  In other words, it doesn't make sense to call
> container_get("/machine") on system emulation mode.

I agree.

> 
> 
>>
>> In this proposal, the qdev_get_machine_unchecked() is only used
>> in user-only mode, part of which shares with system emulation mode
>> (such as device_set_realized, cpu_common_realizefn). The new
>> qdev_get_machine() is only used in system emulation mode and type checking
>> assertion does reduce the irrational use of this function (if any in the
>> future).
> 
> This part confuses me as well.  qdev_get_machine_unchecked() is
> used in both user-only and softmmu, isn't?  Thus we can't say it
> is only used in user-only mode.

You're right about this.

> 
> I think we all agree that qdev_get_machine() should eventually be
> available in softmmu only.

I think we need to make it happen to avoid calling qdev_get_machine()
in user-only mode.

> 
> But I don't think we agree when it would be appropriate to call
> qdev_get_machine_unchecked() instead of qdev_get_machine().
> 
> On both examples in your patch, the code checks for TYPE_MACHINE
> immediately after calling qdev_get_machine_unchecked().  If that
> code is only useful in softmmu mode, when would anybody want to
> call qdev_get_machine_unchecked() in user-only mode?
> 
> 
>>
>> We all agree to use this qdev_get_machine() as little as possible
>> and this patch could make future clean up work easier.
>>
>>>
>>> Once the expectations and use cases are explained, we can choose
>>> a better name for qdev_get_machine_unchecked() and document it
>>> properly.
>>>
>>
> 

Re: [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion

[Eduardo Habkost]

On Thu, Apr 25, 2019 at 11:12:29AM +0800, Like Xu wrote:
> On 2019/4/25 1:21, Eduardo Habkost wrote:
[...]
> > 
> > I think we all agree that qdev_get_machine() should eventually be
> > available in softmmu only.
> 
> I think we need to make it happen to avoid calling qdev_get_machine()
> in user-only mode.

Agreed.  My point is that we we shouldn't need a
qdev_get_machine_unchecked() function at all if we first get rid
of all user-only qdev_get_machine() calls.

> > 
> > But I don't think we agree when it would be appropriate to call
> > qdev_get_machine_unchecked() instead of qdev_get_machine().
> > 
> > On both examples in your patch, the code checks for TYPE_MACHINE
> > immediately after calling qdev_get_machine_unchecked().  If that
> > code is only useful in softmmu mode, when would anybody want to
> > call qdev_get_machine_unchecked() in user-only mode?
> > 
> > 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Apr 25, 2019 at 11:12:29AM +0800, Like Xu wrote:
>> On 2019/4/25 1:21, Eduardo Habkost wrote:
> [...]
>> > 
>> > I think we all agree that qdev_get_machine() should eventually be
>> > available in softmmu only.
>> 
>> I think we need to make it happen to avoid calling qdev_get_machine()
>> in user-only mode.

That would be ideal.

> Agreed.  My point is that we we shouldn't need a
> qdev_get_machine_unchecked() function at all if we first get rid
> of all user-only qdev_get_machine() calls.

Concur.

Re: [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion

[Markus Armbruster]

Like Xu <like.xu@linux.intel.com> writes:

> On 2019/4/18 1:10, Eduardo Habkost wrote:
>> On Wed, Apr 17, 2019 at 07:14:10AM +0200, Markus Armbruster wrote:
>>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>>
>>>> On Mon, Apr 15, 2019 at 03:59:45PM +0800, Like Xu wrote:
>>>>> To avoid the misuse of qdev_get_machine() if machine hasn't been created yet,
>>>>> this patch uses qdev_get_machine_uncheck() for obj-common (share with user-only
>>>>> mode) and adds type assertion to qdev_get_machine() in system-emulation mode.
>>>>>
>>>>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>>>>> Signed-off-by: Like Xu <like.xu@linux.intel.com>
>>>>
>>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>>>>
>>>> I'm queueing the series on machine-next, thanks!
>>>
>>> Hold your horses, please.
>>>
>>> I dislike the name qdev_get_machine_uncheck().  I could live with
>>> qdev_get_machine_unchecked().
>>>
>>> However, I doubt this is the right approach.
>>>
>>> The issue at hand is undisciplined creation of QOM object /machine.
>>>
>>> This patch adds an asseertion "undisciplined creation of /machine didn't
>>> create crap", but only in some places.
>>>
>>> I think we should never create /machine as (surprising!) side effect of
>>> qdev_get_machine().  Create it explicitly instead, and have
>>> qdev_get_machine() use object_resolve_path("/machine", NULL) to get it.
>>> Look ma, no side effects.
>>
>> OK, I'm dropping this one while we discuss it.
>>
>> I really miss a good explanation why qdev_get_machine_unchecked()
>> needs to exist.  When exactly do we want /machine to exist but
>> not be TYPE_MACHINE?  Why?
>
> AFAICT, there is no such "/machine" that is not of type TYPE_MACHINE.
>
> The original qdev_get_machine() would always return a "/container"
> object in user-only mode and there is none TYPE_MACHINE object.
>
> In system emulation mode, it returns the same "/container" object at
> the beginning, until we initialize and add a TYPE_MACHINE object to
> the "/container" as a child and it would return
> OBJECT(current_machine)
> for later usages.

I don't think so.

If you ever call qdev_get_machine() before creating "/machine", you not
only get a bogus "container" object, you *also* set "/machine" to that
object.  When main() later attempts to create the real "/machine", it
fails with "attempt to add duplicate property 'machine' to object (type
'container')", and aborts.  See commit 1a3ec8c1564 and e2fb3fbbf9c.

> The starting point is to avoid using the legacy qdev_get_machine()
> in system emulation mode when we haven't added the "/machine" object.
> As a result, we introduced type checking assertions to avoid premature
> invocations.
>
> In this proposal, the qdev_get_machine_unchecked() is only used
> in user-only mode, part of which shares with system emulation mode
> (such as device_set_realized, cpu_common_realizefn). The new
> qdev_get_machine() is only used in system emulation mode and type
> checking assertion does reduce the irrational use of this function (if
> any in the future).
>
> We all agree to use this qdev_get_machine() as little as possible
> and this patch could make future clean up work easier.

I don't think qdev_get_machine() per se is the problem.  Its side effect
is.  Quoting myself:

    I think we should never create /machine as (surprising!) side effect of
    qdev_get_machine().  Create it explicitly instead, and have
    qdev_get_machine() use object_resolve_path("/machine", NULL) to get it.

>> Once the expectations and use cases are explained, we can choose
>> a better name for qdev_get_machine_unchecked() and document it
>> properly.
>>