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

[Anthony Liguori <anthony@codemonkey.ws>]

Markus Armbruster wrote:
> Anthony Liguori <anthony@codemonkey.ws> 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.

>>> +/* 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 :-)

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

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