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

[Marcel Apfelbaum <marcel.a@redhat.com>]

On Mon, 2014-03-03 at 11:13 +0100, Paolo Bonzini wrote:
> 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. :)
I didn't like it either, but there are still places that use
directly QEMUMachineInitArgs's fields, this is the reason I did
it this way.
 
> 
> 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.
My idea was to tackle the machine-opts first, and be able to replace
all the qemu_opt_get calls with QOM syntax.
This is the reason you see "kernel" in QemuMachineState and not "kernel_filename".
It was easier (at least for me) to handle this transition with 1-1 translation
between the machine option and the QemuMachineState field.

I completely agree with you about moving the fields from QEMUMachineInitArgs
to QemuMachineState, but I would like to handle them as second step (meaning now),
of course I can switch back the field names to the more intuitive ones.
 
I'll think what do to with this patch...

Thanks,
Marcel
> 
> 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);
> >
>