Re: [Qemu-devel] [PATCH 1/4] machine: conversion of QEMUMachineInitArgs to MachineState

[Igor Mammedov <imammedo@redhat.com>]

On Wed,  7 May 2014 17:42:57 +0300
Marcel Apfelbaum <marcel.a@redhat.com> wrote:

> Total removal of QEMUMachineInitArgs struct. QEMUMachineInitArgs's fields
> are copied into MachineState. Removed duplicated fields from MachineState.
> 
> All the other changes are only mechanical refactoring, no semantic changes.
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>    - I am perfectly aware that patches touching a lot of files
>      are not desirable, but this one is a very simple replacement
>      patch:
>        QEMUMachineInitArgs -> MachineState
>        args -> ms
>    - This is the simplest way to get rid of QEMUMachineInitArgs fast.
> 
Patch doesn't apply cleanly on current master

[...]

>  
>  #define PC_Q35_MACHINE_OPTIONS \
> diff --git a/hw/i386/xen_machine_pv.c b/hw/i386/xen_machine_pv.c
> index 9adb57f..fb7a817 100644
> --- a/hw/i386/xen_machine_pv.c
> +++ b/hw/i386/xen_machine_pv.c
> @@ -28,11 +28,11 @@
^^^ file was moved
 
>  #include "xen_domainbuild.h"
>  #include "sysemu/blockdev.h"
>  
> -static void xen_init_pv(QEMUMachineInitArgs *args)
> +static void xen_init_pv(MachineState *ms)
>  {
> -    const char *kernel_filename = args->kernel_filename;
> -    const char *kernel_cmdline = args->kernel_cmdline;
> -    const char *initrd_filename = args->initrd_filename;
> +    const char *kernel_filename = ms->kernel_filename;
> +    const char *kernel_cmdline = ms->kernel_cmdline;
> +    const char *initrd_filename = ms->initrd_filename;
>      DriveInfo *dinfo;
>      int i;
>  
[...]

> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 28f0047..eba0574 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -7,17 +7,10 @@
>  #include "hw/qdev.h"
>  #include "qom/object.h"
>  
> -typedef struct QEMUMachineInitArgs {
> -    const MachineClass *machine;
> -    ram_addr_t ram_size;
> -    const char *boot_order;
> -    const char *kernel_filename;
> -    const char *kernel_cmdline;
> -    const char *initrd_filename;
> -    const char *cpu_model;
> -} QEMUMachineInitArgs;
>  
> -typedef void QEMUMachineInitFunc(QEMUMachineInitArgs *args);
> +typedef struct MachineState MachineState;
> +
> +typedef void QEMUMachineInitFunc(MachineState *ms);
>  
>  typedef void QEMUMachineResetFunc(void);
>  
> @@ -61,8 +54,6 @@ int qemu_register_machine(QEMUMachine *m);
>  #define MACHINE_CLASS(klass) \
>      OBJECT_CLASS_CHECK(MachineClass, (klass), TYPE_MACHINE)
>  
> -typedef struct MachineState MachineState;
> -
>  MachineClass *find_default_machine(void);
>  extern MachineState *current_machine;
>  
> @@ -79,7 +70,7 @@ struct MachineClass {
>      const char *alias;
>      const char *desc;
>  
> -    void (*init)(QEMUMachineInitArgs *args);
> +    void (*init)(MachineState *state);
>      void (*reset)(void);
>      void (*hot_add_cpu)(const int64_t id, Error **errp);
>      int (*kvm_type)(const char *arg);
> @@ -111,9 +102,6 @@ struct MachineState {
>      char *accel;
>      bool kernel_irqchip;
>      int kvm_shadow_mem;
> -    char *kernel;
> -    char *initrd;
> -    char *append;
>      char *dtb;
>      char *dumpdtb;
>      int phandle_start;
> @@ -123,7 +111,13 @@ struct MachineState {
>      bool usb;
>      char *firmware;
>  
> -    QEMUMachineInitArgs init_args;
> +    const MachineClass *machine;
Why not drop ^^^? User should use MACHINE_GET_CLASS() instead.
Like this:

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 4fbdaaa..1bedb09 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -145,9 +145,10 @@ static void pc_init1(MachineState *ms,
     guest_info->isapc_ram_fw = !pci_enabled;
 
     if (smbios_defaults) {
+        MachineClass *mc = MACHINE_GET_CLASS(ms);
         /* These values are guest ABI, do not change */
         smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
-                            ms->machine->name, smbios_legacy_mode);
+                            mc->name, smbios_legacy_mode);
     }
 
     /* allocate ram and load rom/bios */
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 315cfe5..df13a9e 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -132,9 +132,10 @@ static void pc_q35_init(MachineState *ms)
     guest_info->has_acpi_build = has_acpi_build;
 
     if (smbios_defaults) {
+        MachineClass *mc = MACHINE_GET_CLASS(ms);
         /* These values are guest ABI, do not change */
         smbios_set_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
-                            ms->machine->name, smbios_legacy_mode);
+                            mc->name, smbios_legacy_mode);
     }
 
     /* allocate ram and load rom/bios */
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 789a0c2..b62de4a 100644
--- a/include//
+++ b/include/hw/boards.h
@@ -112,7 +112,6 @@ struct MachineState {
     bool usb;
     char *firmware;
 
-    const MachineClass *machine;
     ram_addr_t ram_size;
     const char *boot_order;
     const char *kernel_filename;
diff --git a/vl.c b/vl.c
index 211efec..2de90fb 100644
--- a/vl.c
+++ b/vl.c
@@ -4420,7 +4420,6 @@ int main(int argc, char **argv, char **envp)
 
     qdev_machine_init();
 
-    current_machine->machine = machine_class;
     current_machine->ram_size = ram_size;
     current_machine->boot_order = boot_order;
     current_machine->kernel_filename = kernel_filename;


> +    ram_addr_t ram_size;
> +    const char *boot_order;
> +    const char *kernel_filename;
> +    const char *kernel_cmdline;
> +    const char *initrd_filename;
> +    const char *cpu_model;
>  };
>  
>  #endif

I've reviewed all targets, it looks good.
+1 to afaerber's comment s/ms/machine/ 

-- 
Regards,
  Igor