From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46164) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VBLhG-0001Fg-6x for qemu-devel@nongnu.org; Mon, 19 Aug 2013 05:24:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VBLhA-0001ZY-LG for qemu-devel@nongnu.org; Mon, 19 Aug 2013 05:24:22 -0400 From: Markus Armbruster References: <1376651630-9151-1-git-send-email-armbru@redhat.com> <1376651630-9151-6-git-send-email-armbru@redhat.com> <520F548C.2090903@redhat.com> Date: Mon, 19 Aug 2013 11:24:10 +0200 In-Reply-To: <520F548C.2090903@redhat.com> (Laszlo Ersek's message of "Sat, 17 Aug 2013 12:46:36 +0200") Message-ID: <87pptadoed.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v4 5/6] ppc: Don't duplicate QEMUMachineInitArgs in PPCE500Params List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laszlo Ersek Cc: aliguori@us.ibm.com, mjt@tls.msk.ru, qemu-devel@nongnu.org, agraf@suse.de, blauwirbel@gmail.com, qemu-ppc@nongnu.org, aviksil@linux.vnet.ibm.com Laszlo Ersek writes: > comments below > > On 08/16/13 13:13, armbru@redhat.com wrote: >> From: Markus Armbruster >> >> Pass on the generic arguments unadulterated, and the machine-specific >> ones as separate argument. >> >> Signed-off-by: Markus Armbruster >> Acked-by: Alexander Graf >> --- >> hw/ppc/e500.c | 35 ++++++++++++++++++----------------- >> hw/ppc/e500.h | 13 +++---------- >> hw/ppc/e500plat.c | 8 +------- >> hw/ppc/mpc8544ds.c | 8 +------- >> 4 files changed, 23 insertions(+), 41 deletions(-) > > Please always use > > -O/path/to/order_file > > when invoking git-format-patch. > > The contents of "order_file" should be minimally > > configure > Makefile* > *.json > *.h > *.c > > It's much easier to review a patch when "declarative changes" are shown > first (ie. in approximate logical dependency order). Is there a way to put this in .git/config? Should http://wiki.qemu.org/Contribute/SubmitAPatch ask for this? > Then, > >> -void ppce500_init(PPCE500Params *params) >> +void ppce500_init(QEMUMachineInitArgs *args, PPCE500Params *params) >> { >> MemoryRegion *address_space_mem = get_system_memory(); >> MemoryRegion *ram = g_new(MemoryRegion, 1); >> @@ -584,8 +585,8 @@ void ppce500_init(PPCE500Params *params) >> PPCE500CCSRState *ccsr; >> >> /* Setup CPUs */ >> - if (params->cpu_model == NULL) { >> - params->cpu_model = "e500v2_v30"; >> + if (args->cpu_model == NULL) { >> + args->cpu_model = "e500v2_v30"; >> } > > As discussed before, this change will modify the "args.cpu_model" member > in main(), but that's OK. > > >> @@ -634,7 +635,7 @@ void ppce500_init(PPCE500Params *params) >> >> /* Fixup Memory size on a alignment boundary */ >> ram_size &= ~(RAM_SIZES_ALIGN - 1); >> - params->ram_size = ram_size; >> + args->ram_size = ram_size; > > This hackery (commendably left intact by the patch) is convincing me > that QEMUMachineInitArgs should not have a "ram_size" member at all. If > "ram_size" is a well-founded global, then let's treat it as such. Whatever. Global variables are often bad style (there are exceptions). Even worse is passing what is essentially global state down call chains while keeping the global variables around for random poking. And that's what we tend to do %-/ > Reviewed-by: Laszlo Ersek Thanks!