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

[Markus Armbruster]

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.

>> +/* 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?

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.

> I'm not sure this structure is going to scale well as we introduce
> more types of host devices.  You don't necessarily need to address the
> host configuration file part of this at this stage.

Agreed.  struct dt_host was just the simplest way I could find to
separate machine from host configuration, and still keep host
configuration together in one place.  I rather like to have it in one
place right now, because it makes it easier for me to find out what host
configuration actually is.

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

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

>> +static dt_driver *
>> +dt_driver_by_name(const char *name)
>>   
>
> While I'm not wildly opposed to this style (it's nice for grepping),
> most of the rest of the code doesn't do this (it keeps it on the same
> line).

Done.

> Regards,
>
> Anthony Liguori

Thanks!

-- 
Consistently separating words by spaces became a general custom about
the tenth century A.D., and lasted until about 1957, when FORTRAN
abandoned the practice.
        -- Sun FORTRAN Reference Manual