Re: [Qemu-devel] Machine description as data prototype, take 3

[Markus Armbruster]

Anthony Liguori <address@hidden> writes:

> Markus Armbruster wrote:
>> Anthony Liguori <address@hidden> writes:
>>
>>   
>>>> diff --git a/hw/dt.c b/hw/dt.c
>>>> +
>>>>         
>>> Please remove the ^Ls.  They don't render properly in my mail client.
>>>     
>>
>> Many source files contain ^L already.  But I'll drop mine if you insist.
>>   
>
> My mailer (Thunderbird) expands the ^L into about 20 new lines making
> the patches difficult to review.

I'll drop them.  Pity, since they're quite useful for navigating.

>>>> +/* Host Configuration */
>>>> +
>>>> +typedef struct dt_host {
>>>> +    /* connection NICs <-> VLAN */
>>>> +    tree *nic[MAX_NICS];
>>>> +    VLANState *nic_vlan[MAX_NICS];
>>>> +    /* connection drives <-> controller */
>>>> +    tree *drive_ctrl[MAX_DRIVES];
>>>> +    BlockDriverState *drive_state[MAX_DRIVES];
>>>> +} dt_host;
>>>>
>>>>         
>>> typedef struct DeviceTreeHost
>>> {
>>> } DeviceTreeHost.
>>>     
>>
>> If you insist on CamelCase, IFindThatUglyAndHardToRead, but I can do
>> that.  Just typedef names?
>>   
>
> Why?  Just convert to the way the rest of the code does it.
> Conformity feels good, I promise :-)

But what does the rest of the code actually do?  I can see plenty of
CamelTypeNames, but functions?

Out of curiosity, I used ctags -x to get me separate lists of distinct
function, typedef, variable, member and struct/union/enum tag
definitions (macros and enumerators omitted):

                        function  typedef   variable  member    tags
lower_case [a-z0-9_]     10958       589      2226     10385       813
CamelCase  [a-zA-Z0-9]     164       415       111       249         0
other                      625        65        44       123       318
total                    11747      1069      2381     10757      1131

Naturally, names for widely used, important stuff matter more.  Whether
that tips the balance for typedef names towards CamelCase I don't know
and don't wish to argue about, I'll just do as you wish.  But for
everything else, the conformity argument seems to support lower_case.

>> As to the placement of braces, a quick grep shows the vast majority of
>> such typedefs to have the brace on the same line as the typedef.
>>   
>
> Yeah, that's fine.  My bracket placement was not intentional.
>
>>> For instance, I think it would be perfectly fine to require to start
>>> with that the command line configuration matches the describe machine
>>> file.  For instance, if you see:
>>>
>>> -net tap -net nic,model=rtl8139
>>>
>>> Then you should search for an rtl8139 and configure the node to be on
>>> vlan=0.  If an rtl8139 doesn't exist, throw an error.
>>>     
>>
>> Conversely, when an optional tree node isn't enabled (e.g. with -net nic
>> for NICs), silently cut it from the tree.
>>   
>
> I'd prefer the first iteration to not modify the tree at all.  The
> specified command line configuration should exactly match whatever the
> tree specifies.
>
> How manipulate the tree in a generic way is IMHO a harder problem that
> deserves to be addressed in a proper way.

I agree that we shouldn't attempt to solve that problem in the first
iteration.

Until we solve it, we need a stub.  Whether that stub edits the tree or
not seems relatively unimportant to me, as long as it is cleanly
encapsulated.

>>> The long term goal, would be to have a mechanism to modify the tree in
>>> a generic way and the -net nic code would end up looking like:
>>>
>>> node = find_next_device("type=nic,model=rtl8139");
>>> if (!node) {
>>>   node = find_bus("type=pcibus");
>>>   if (!node)
>>>       bail out
>>>   node = add_node_to_bus(node,
>>> "type=nic,model=rtl8139,remaining_description_of_rtl8139");
>>>   if (!node)
>>>       bail out
>>> }
>>>
>>> attach_nic_to_vlan(vlan, node);
>>>     
>>
>> Makes sense to me.
>>
>> The driver should declare on what kind(s) of bus this device can go.
>>   
>
> Yup.
>
> Regards,
>
> Anthony Liguori

Thanks again!