Date: Tue, 2 Sep 2008 09:44:50 -0500 From: Brooks Davis <brooks@freebsd.org> To: Julian Elischer <julian@elischer.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: <20080902144450.GC48622@lor.one-eyed-alien.net> In-Reply-To: <48BD4EBE.4010005@elischer.org> References: <20080828185639.P66593@maildrop.int.zabbadoz.net> <20080902000516.GA48622@lor.one-eyed-alien.net> <48BCED8E.5030109@elischer.org> <20080902141245.GB48622@lor.one-eyed-alien.net> <48BD4EBE.4010005@elischer.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--m51xatjYGsM+13rf Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Sep 02, 2008 at 07:33:34AM -0700, Julian Elischer wrote: > Brooks Davis wrote: >> 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, >>>>>=20 >>>>> 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: >>>>>=20 >>>>> - 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... >>>>>=20 >>>>> 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: >>>>=20 >>>> - 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 thi= s.. >>>=20 >>> 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.. >>>=20 >>> I suggest that we eventually replace: >>>=20 >>> VNET_ITERLOOP_BEGIN >>> stuff >>> VNET_ITERLOOP_END >>>=20 >>>=20 >>> with (eventually) >>>=20 >>> FOREACH_VNET(vnet) { >>> stuff >>> } >>>=20 >>> but that would require that the entire contents of "stuff" >>> would appear in the diff. >>=20 >> Thinking about it more, at a minimum, I think we should do: >> VNET_ITERLOOP_BEGIN >> stuff >> VNET_ITERLOOP_END >=20 > when? (i.e. at which stage)? >=20 >=20 > and doing FOREACH_VNET() {} > will allow brace matching... Before the final. We probably don't need to hold this commit up for it, but we should make the commit seperate from the final one. If we could switch to FOREACH_VNET() {} soon, that would be good. -- Brooks >>>> - 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? >>>=20 >>> not sure myself.. I've been only following the tty mashup from a distan= ce. >>>=20 >>>> - 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. >>>=20 >>> teh original code actually includes vnet.h directly in about 50 source= =20 >>> files. >>>=20 >>> my attempt to include it from if.h cut that down to 3. >>>=20 >>> 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. >>=20 >> 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). >>=20 >> -- Brooks >=20 --m51xatjYGsM+13rf Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.8 (FreeBSD) iD8DBQFIvVFhXY6L6fI4GtQRAipYAJ4h+7Yc56BT076ZhNefSEk0+ChiOQCfahLj Tkvm0spGsLbjIifAAgTG/mE= =2H1E -----END PGP SIGNATURE----- --m51xatjYGsM+13rf--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080902144450.GC48622>