Date: Tue, 02 Sep 2008 00:38:54 -0700 From: Julian Elischer <julian@elischer.org> To: Brooks Davis <brooks@freebsd.org> Cc: "Bjoern A. Zeeb" <bz@freebsd.org>, FreeBSD virtualization mailing list <freebsd-virtualization@freebsd.org> Subject: Re: Step 1.5 needs review Message-ID: <48BCED8E.5030109@elischer.org> In-Reply-To: <20080902000516.GA48622@lor.one-eyed-alien.net> References: <20080828185639.P66593@maildrop.int.zabbadoz.net> <20080902000516.GA48622@lor.one-eyed-alien.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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. > - 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. > - sys/netinet/ip_fw.h: > - It's not clear to me that all the changes in this file should > actually be here. It looks like there may be some merge issues or > WIP code. I'll have a look. thanks for the pointer. > - sys/netinet6/icmp6.c: > - This (and IIRC a couple other files in ipsec) contain comments like > "XXX this below is WRONG". It would be nice if the issue could be > fixed or if that's not feasible, a more detailed comment should > be added. hmm yeah > - sys/netipsec/ipsec.c: > - This and a few other files change the style of SYSCTL*() macro > usage. We're not at all consistent across the kernel, but my > feeling is that we should either entirely preserve the local style > or we should figure out what the style should be and do the churn > now while we have to mangle the wrapping anyway. > I can do that (or you can... just run the update.sh script at the base..) > > -- Brooks
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?48BCED8E.5030109>