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 <marce...@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);
> >
>
