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

[Laszlo Ersek]

On 08/19/13 11:24, Markus Armbruster wrote:
> Laszlo Ersek <address@hidden> writes:

>> 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?

The only way we've found thus far is to introduce a new alias for
git-format-patch that hardwires it.

> Should http://wiki.qemu.org/Contribute/SubmitAPatch ask for this?

Not a bad idea, but I'm afraid new contributors don't read it because
they don't know about it, and veteran contributors don't read it because
they don't need it.

The wiki would need a usable table of contents anyway, I have great
trouble every time I want to find anything. Wikipedia probably should
not have a flat sitemap for its millions of articles. The qemu wiki
should, for its handful.

Laszlo

> 
>> 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!
>