Re: [Qemu-devel] [PATCH v4 5/6] ppc: Don't duplicate QEMUMachineInitArgs in PPCE500Params

[Markus Armbruster]

Laszlo Ersek <address@hidden> writes:

> comments below
>
> On 08/16/13 13:13, address@hidden wrote:
>> From: Markus Armbruster <address@hidden>
>> 
>> Pass on the generic arguments unadulterated, and the machine-specific
>> ones as separate argument.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> Acked-by: Alexander Graf <address@hidden>
>> ---
>>  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 <address@hidden>

Thanks!