Message ID | 1393765632-2753-10-git-send-email-marcel.a@redhat.com |
---|---|
State | New |
Headers | show |
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); >
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 --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);
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(-)