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

[Anthony Liguori]

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.

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