Step 1.5 needs review

[Brooks Davis]

On Tue, Sep 02, 2008 at 12:38:54AM -0700, Julian Elischer wrote:
> Brooks Davis wrote:
>> On Thu, Aug 28, 2008 at 07:03:30PM +0000, Bjoern A. Zeeb wrote:
>>> Hi,
>>> 
>>> in case you are interested or have volunteered before to review Step 1.5
>>> as described on   http://wiki.freebsd.org/Image/Notes200808DevSummit
>>> there are few things to do:
>>> 
>>> - review the diff (Julian posted an initial one).
>>> - make sure all (relevant) sysctl were caught.
>>> - make sure the INIT_VNET_* macro is there whereever it is needed.
>>> - do builds according to "HOWTO verify that the pure style changes are
>>>   all right" on the above mentioned page and verify that it is all
>>>   style changes. In case there are others we shoudl decide to either
>>>   commit them either upfront or afterwards if possible.
>>> - the 'include headers' one way or the other (as we have discussed at
>>>   the devsummit and that Julian has told me again) needs resolving.
>>>   As this has bikeshed potential, I'd prefer that the 'singed up'
>>>   reviewers decide that.
>>> - possibly more...
>>> 
>>> The plan would be to have a final patch by Monday morning UTC to be
>>> comitted by a volunteer.
>> 
>> I've gone over the patch and fixed some white space issues.  I've also
>> found some things I'm not sure what to do with.  Comments:
>> 
>> - GENERIC_NODEBUG should not be committed
>> - VNET_ITERLOOP_BEGIN/END is evil.  It would be really nice to find a
>>   way to do this that preserves {} pairs and isn't too magic.
> 
> The requirement is to take soem code that doesn something once.
> and do it once for each vimage. There are of course many ways to do this..
> 
> Once we have the code in, I think we should expand this out
> and correctly indent the code, but for reasons of "minimum diff size"
> teh current way seems ok to me though it doens't look pretty..
> 
> I suggest that we eventually replace:
> 
> 	VNET_ITERLOOP_BEGIN
> 	stuff
> 	VNET_ITERLOOP_END
> 
> 
> with (eventually)
> 
> 	FOREACH_VNET(vnet) {
> 		stuff
> 	}
> 
> but that would require that the entire contents of "stuff"
> would appear in the diff.

Thinking about it more, at a minimum, I think we should do:
	VNET_ITERLOOP_BEGIN
		stuff
	VNET_ITERLOOP_END

>> - sys/kern/tty.c:
>>   - There's some #if 0 code that presumably should stay in the vimage
>>     branch for now and be fixed before the final commit.
>>   - TIOCDRAIN is being removed.  Is this a merge issue or something
>>     else?
> 
> 
> not sure myself.. I've been only following the tty mashup from a distance.
> 
>> - sys/net/if.h:
>>   - shouldn't net/vnet.h be included in if_var.h instead?  *_var.h is
>>     supposed to be the internals and I think this qualifies.  If so,
>>     there will be a number of files that added if.h includes that
>>     should add if_var.h includes instead.
> 
> I actually looked around to find a good place to icnlude vnet.h from
> and decided on if.h because it seemt o be included almist everywhere
> that vnet.h needed to be included, but I'm not religious on it.
> 
> teh original code actually includes vnet.h directly in about 50 source 
> files.
> 
> my attempt to include it from if.h cut that down to 3.
> 
> I'm not sure I want to actually include the contents directly into
> if.h  or any other place.. I think keeping a separate vnet.h and
> vinet.h seems ok to me.

The #ifdef _KERNEL is a strong hint that it belongs in if_var.h if it's
going to be included in another header (IMO, the vnet/vinet.h files
aren't a good idea in the long term).

-- Brooks
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-virtualization/attachments/20080902/9c53ca64/attachment.pgp