From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58099) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKRh7-0004oZ-W0 for qemu-devel@nongnu.org; Mon, 03 Mar 2014 07:10:11 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WKRgy-0005iU-OB for qemu-devel@nongnu.org; Mon, 03 Mar 2014 07:10:05 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55373) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKRgy-0005iN-GG for qemu-devel@nongnu.org; Mon, 03 Mar 2014 07:09:56 -0500 Message-ID: <1393848606.3882.7.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Mon, 03 Mar 2014 14:10:06 +0200 In-Reply-To: <531455BB.70907@redhat.com> References: <1393765632-2753-1-git-send-email-marcel.a@redhat.com> <1393765632-2753-10-git-send-email-marcel.a@redhat.com> <531455BB.70907@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RFC V2 9/9] hw/core: mapped QemuOpts into QEMUMachineInitArgs fields to remove duplication List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: peter.maydell@linaro.org, blauwirbel@gmail.com, mdroth@linux.vnet.ibm.com, mst@redhat.com, armbru@redhat.com, mtosatti@redhat.com, agraf@suse.de, ehabkost@redhat.com, qemu-devel@nongnu.org, peter.crosthwaite@petalogix.com, quintela@redhat.com, aliguori@amazon.com, imammedo@redhat.com, scottwood@freescale.com, edgar.iglesias@gmail.com, lcapitulino@redhat.com, afaerber@suse.de, rth@twiddle.net 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 > > 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); > > >