From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35865) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKPsL-0008II-Jm for qemu-devel@nongnu.org; Mon, 03 Mar 2014 05:13:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WKPsB-0002ia-2h for qemu-devel@nongnu.org; Mon, 03 Mar 2014 05:13:33 -0500 Received: from mail-qc0-x22a.google.com ([2607:f8b0:400d:c01::22a]:46871) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKPsA-0002iU-Sj for qemu-devel@nongnu.org; Mon, 03 Mar 2014 05:13:23 -0500 Received: by mail-qc0-f170.google.com with SMTP id c9so3583861qcz.29 for ; Mon, 03 Mar 2014 02:13:22 -0800 (PST) Sender: Paolo Bonzini Message-ID: <531455BB.70907@redhat.com> Date: Mon, 03 Mar 2014 11:13:15 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1393765632-2753-1-git-send-email-marcel.a@redhat.com> <1393765632-2753-10-git-send-email-marcel.a@redhat.com> In-Reply-To: <1393765632-2753-10-git-send-email-marcel.a@redhat.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed 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: Marcel Apfelbaum , qemu-devel@nongnu.org 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, lcapitulino@redhat.com, peter.crosthwaite@petalogix.com, quintela@redhat.com, aliguori@amazon.com, imammedo@redhat.com, scottwood@freescale.com, edgar.iglesias@gmail.com, afaerber@suse.de, rth@twiddle.net 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. :) 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); >