diff mbox

[RFC,V2,9/9] hw/core: mapped QemuOpts into QEMUMachineInitArgs fields to remove duplication

Message ID 1393765632-2753-10-git-send-email-marcel.a@redhat.com
State New
Headers show

Commit Message

Marcel Apfelbaum March 2, 2014, 1:07 p.m. UTC 
The QOM machine has both field per QemuOpt and an instance of
QEMUMachineInitArgs. Removed dupplications.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 hw/core/machine.c   | 18 +++++++++---------
 include/hw/boards.h |  9 +++------
 vl.c                |  6 +++---
 3 files changed, 15 insertions(+), 18 deletions(-)

Comments

Paolo Bonzini March 3, 2014, 10:13 a.m. UTC | #1 
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);
>
Marcel Apfelbaum March 3, 2014, 12:10 p.m. UTC | #2 
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);
> >
>
diff mbox

Patch

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