Re: [Qemu-devel] [PATCH RFC V2 9/9] hw/core: mapped QemuOpts into QEMUMachineInitArgs fields to remove duplication

[Paolo Bonzini <pbonzini@redhat.com>]

Il 02/03/2014 14:07, Marcel Apfelbaum ha scritto:
> The QOM machine has both field per QemuOpt and an instance of
> QEMUMachineInitArgs. Removed dupplications.
>
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>

I think this patch is a bit backwards. :)

You should _start_ with properties that access QEMUMachineInitArgs 
fields, and nothing like the "char *kernel" fields you currently have in 
QemuMachineState.  Then, as part of a patch that does a global 
s/QEMUMachineInitArgs/QemuMachineState/, you move the fields from 
QEMUMachineInitArgs to QemuMachineState, and remove the "init_args." 
indirection in the property accessors.

BTW, it is much simpler if you do not change the field names in the 
process: keep kernel_filename in QemuMachineState, do not change it to 
kernel.

Paolo

> ---
>  hw/core/machine.c   | 18 +++++++++---------
>  include/hw/boards.h |  9 +++------
>  vl.c                |  6 +++---
>  3 files changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 436829c..73f61bd 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -67,37 +67,37 @@ static void machine_set_kvm_shadow_mem(Object *obj, Visitor *v,
>  static char *machine_get_kernel(Object *obj, Error **errp)
>  {
>      QemuMachineState *machine_state = QEMU_MACHINE(obj);
> -    return g_strdup(machine_state->kernel);
> +    return g_strdup(machine_state->init_args.kernel_filename);
>  }
>
>  static void machine_set_kernel(Object *obj, const char *value, Error **errp)
>  {
>      QemuMachineState *machine_state = QEMU_MACHINE(obj);
> -    machine_state->kernel = g_strdup(value);
> +    machine_state->init_args.kernel_filename = g_strdup(value);
>  }
>
>  static char *machine_get_initrd(Object *obj, Error **errp)
>  {
>      QemuMachineState *machine_state = QEMU_MACHINE(obj);
> -    return g_strdup(machine_state->initrd);
> +    return g_strdup(machine_state->init_args.initrd_filename);
>  }
>
>  static void machine_set_initrd(Object *obj, const char *value, Error **errp)
>  {
>      QemuMachineState *machine_state = QEMU_MACHINE(obj);
> -    machine_state->initrd = g_strdup(value);
> +    machine_state->init_args.initrd_filename = g_strdup(value);
>  }
>
>  static char *machine_get_append(Object *obj, Error **errp)
>  {
>      QemuMachineState *machine_state = QEMU_MACHINE(obj);
> -    return g_strdup(machine_state->append);
> +    return g_strdup(machine_state->init_args.kernel_cmdline);
>  }
>
>  static void machine_set_append(Object *obj, const char *value, Error **errp)
>  {
>      QemuMachineState *machine_state = QEMU_MACHINE(obj);
> -    machine_state->append = g_strdup(value);
> +    machine_state->init_args.kernel_cmdline = g_strdup(value);
>  }
>
>  static char *machine_get_dtb(Object *obj, Error **errp)
> @@ -261,9 +261,9 @@ static void qemu_machine_finalize(Object *obj)
>      QemuMachineState *machine_state = QEMU_MACHINE(obj);
>
>      g_free(machine_state->accel);
> -    g_free(machine_state->kernel);
> -    g_free(machine_state->initrd);
> -    g_free(machine_state->append);
> +    g_free(machine_state->init_args.kernel_filename);
> +    g_free(machine_state->init_args.initrd_filename);
> +    g_free(machine_state->init_args.kernel_cmdline);
>      g_free(machine_state->dtb);
>      g_free(machine_state->dumpdtb);
>      g_free(machine_state->dt_compatible);
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 053c113..4b1979e 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -13,9 +13,9 @@ typedef struct QEMUMachineInitArgs {
>      const QEMUMachine *machine;
>      ram_addr_t ram_size;
>      const char *boot_order;
> -    const char *kernel_filename;
> -    const char *kernel_cmdline;
> -    const char *initrd_filename;
> +    char *kernel_filename;
> +    char *kernel_cmdline;
> +    char *initrd_filename;
>      const char *cpu_model;
>  } QEMUMachineInitArgs;
>
> @@ -91,9 +91,6 @@ struct QemuMachineState {
>      char *accel;
>      bool kernel_irqchip;
>      int kvm_shadow_mem;
> -    char *kernel;
> -    char *initrd;
> -    char *append;
>      char *dtb;
>      char *dumpdtb;
>      int phandle_start;
> diff --git a/vl.c b/vl.c
> index 007342c..3d7357e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2834,8 +2834,8 @@ int main(int argc, char **argv, char **envp)
>      int i;
>      int snapshot, linux_boot;
>      const char *icount_option = NULL;
> -    const char *initrd_filename;
> -    const char *kernel_filename, *kernel_cmdline;
> +    char *initrd_filename;
> +    char *kernel_filename, *kernel_cmdline;
>      const char *boot_order;
>      DisplayState *ds;
>      int cyls, heads, secs, translation;
> @@ -4129,7 +4129,7 @@ int main(int argc, char **argv, char **envp)
>      }
>
>      if (!kernel_cmdline) {
> -        kernel_cmdline = "";
> +        kernel_cmdline = (char *)"";
>      }
>
>      linux_boot = (kernel_filename != NULL);
>