From owner-freebsd-virtualization@FreeBSD.ORG Wed Jun 11 14:56:43 2008 Return-Path: Delivered-To: freebsd-virtualization@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 81D201065670; Wed, 11 Jun 2008 14:56:43 +0000 (UTC) (envelope-from zec@freebsd.org) Received: from xaqua.tel.fer.hr (xaqua.tel.fer.hr [161.53.19.25]) by mx1.freebsd.org (Postfix) with ESMTP id DE5A18FC12; Wed, 11 Jun 2008 14:56:42 +0000 (UTC) (envelope-from zec@freebsd.org) Received: by xaqua.tel.fer.hr (Postfix, from userid 20006) id 5644D9B653; Wed, 11 Jun 2008 16:23:25 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.1.7 (2006-10-05) on xaqua.tel.fer.hr X-Spam-Level: X-Spam-Status: No, score=-3.9 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.1.7 Received: from [192.168.200.111] (zec2.tel.fer.hr [161.53.19.79]) by xaqua.tel.fer.hr (Postfix) with ESMTP id 186589B652; Wed, 11 Jun 2008 16:23:17 +0200 (CEST) From: Marko Zec To: "Bjoern A. Zeeb" Date: Wed, 11 Jun 2008 16:22:58 +0200 User-Agent: KMail/1.9.7 References: <484CC690.9020303@elischer.org> <20080609174826.Q83875@maildrop.int.zabbadoz.net> In-Reply-To: <20080609174826.Q83875@maildrop.int.zabbadoz.net> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200806111622.58194.zec@freebsd.org> X-Mailman-Approved-At: Wed, 11 Jun 2008 15:36:30 +0000 Cc: freebsd-virtualization@freebsd.org Subject: Re: kinda headsup.. X-BeenThere: freebsd-virtualization@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Discussion of various virtualization techniques FreeBSD supports." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 11 Jun 2008 14:56:43 -0000 On Monday 09 June 2008 21:24:42 Bjoern A. Zeeb wrote: > On Sun, 8 Jun 2008, Julian Elischer wrote: > > At the BSDCAn devsummit we discussed how to proceed with committing > > Vimage to -current. > > > > the Milestones included something like: > > > > June 8 (today) Headsup.... > > > > June 15 commit changes that add macros for vnet > > (network module) and vinet(inet virtualisation) > > with macros defined in such a way to make 0 actual > > differences. provable by md5 etc. > > Documentat > > s/hostname/g//V_hostname/ > > #define V_hostname hostname > > 2 weeks settle time, next step prepared, tested > > and reviewed. > > For which part were you talking about a sed/awk script to use? > Can we have a diff for just this part (once it is avail?) There's now a script in p4 projects/vimage/var_rename.tcl - that one is done in TCL, doing it in sed/awk was easier said than done... In projects/vimage/misc/ there's an machine-generated diff plus another small one manually forged that has to be applied afterwards for clean compile. > [schedule] > > * I am missing the BIG HEADS UP somewhere for all the people with > outstanding work so that they will not re-do any integration multiple > times. > > * I am missing the developers and users documentation in the > schedule. Users doc: vimage(8) is not entirely in sync with most recent code but not too misleading either, and there's a reasonably up-to-date cookbook here: http://imunes.net/virtnet/eurobsdcon07_tutorial.pdf Developers doc: yes I know it's a showstopper, it's in a pipeline... > > diffs can be found at: > > http://www.freebsd.org/~julian/vimage.diff and it are usually > > fairly up to date. > > I am just starting to skip through the patch, not doing a close > review atm (not checking functional changes, etc. at all), and even > this is hard at the end of the day... > > Are sys/ddb/db_command.c related in any way to this? No, the change there merely displays the list of possible command completions in a sorted order. This is nothing vimage-specific. > sys/kern/init_main.c has an extra whitespace before the LIST_FIRST. OK. Thanks for reporting this and others style violations bellow, will do a sweeping pass over the vimage branch today. > sys/kern/kern_linker.c Isizeof(lookup) should be 4 space indent not 2 > tabs. > > Do we need those changes like sys/kern/kern_switch.c ? Not really, they were / are experimental, primarily to check out how the virtualization framework can deal with other subsystems unrelated to networking. There's no plan to push for commiting these bits. > sys/kern/kern_sysctl.c has indentation problems in the > @@ -1322,7 +1421,17 @@ junk > > sys/kern/kern_timeout.c has an extra whitespace > > sys/kern/kern_vimage.c says "XXX RCS tag goes here" so add it. > sys/kern/kern_vimage.c has // comments no-no > - " - s,#define NAME,#define\tNAME,g > - " - vnet_mod_register,vnet_mod_register_multi,(more)... > declarations - " - adds a new suser() call. Yes priv() should be used here instead. > - " - in vi_symlookup() 2nsd line of for, remove a space > - " - thinks like this scare me: > /* A brute force check whether there's enough mem for a new vimage > */ especially if its freed again instantly This is a huge problem that needs much work. In general if any kernel subsystem fails to allocate resources at boot time it typically panics. With vimage at any point in time we might be calling those same per-subsystem initialization vectors at run time, but with a running system we need a more gratitious back-out mechanism in resource shortages than panics. So all of the existing initialization functions will have to be extended to potentially return an error, and we'll have to extend the virtualization framework to roll back partially initialized vnets in cases of such failures. > - " - near Detach / free per-module state instances remove > whitespace - " - vi_free() remove a \t before the break; > - " - db_show_vnets should probably check for db_pager_quit OK > sys/kern/kern_xxx.c the printf looks like debugging? yup > sys/kern/sys_socket.c has an unrelated whitespace change > > sys/kern/uipc_domain.c removes a comment I am entirely sure it can be > removed. - " - why do we need to change net_init_domain(?here?) just > to cast again? OK the original comment can be restored. Casting: net_init_domain() is now of vnet_attach_fn type which passes generic void *arg from the caller, this arg may be something other than struct domain * in other cases. The idea is that we can register a single initialization function multiple times with different arguments (different struct domain * in this case). When a new vnet gets instantiated, the vimage framework takes care to invoke each registration of net_init_domain() with proper struct domain *arg in the proper order. > sys/kern/uipc_socket.c junk @@ -1284,13 +1314,17 @@ s,\t, , > - " - if (how != SHUT_RD) { int error; add \n OK true > sys/kern/vfs_lookup.c adds something called IMUNES_SYMLINK_HACK which > should either be renamed or removed. Yes as the name implies this is a HACK not intended to be commited. > sys/modules/Makefile does not look like it belongs there Hm I had some issues compiling zfs module long time ago so disabled this, will see if zfs + VIMAGE can be compiled now. > sys/modules/netgraph/Makefile looks really strange, can we fix > that? This was my best shot hack at compiling ng_wormhole only if options VIMAGE is defined. ng_wormhole makes no sense and doesn't even compile on non-vimage kernels - it provides an explicit "tunnel" from one vnet to another at negraph layer. > sys/modules/netgraph/pipe/Makefile has an extra space > > sys/modules/netgraph/wormhole/Makefile has an extra space > > sys/net/bpf.c adds an IMUNES_BPF_HACK, and defines it - either > rename or remove; also has whitespace issues and debugging > printfs in there (that should not compile). A HACK -> not to be commited... > sys/net/if.c @@ -292,31 +317,73 @@ junk if (IS_DEFAULT_VNET(curvnet)) > { ... needs an extra \t, no? doesn't look nice; there are more of > those in this file; maybe not yet; not before the #ifdefs go. - " - > SYSINT .. if_attachdomain was a wrong ws change > - " - junk @@ -1842,6 +1971,24 @@ adds another suser() > - " - at the end there are two unrelated/wrong ws changes > > sys/net/if_ethersubr.c ether_reassign() has whitespace issues > - " - SYSCTL_V_INT for ether_ipfw 2nd line indent looks wrong > > sys/net/if_gif.c SYSCTL_V_INT 2nd line, parallel_tunnels indent > - " - gifmodevent() empty line wrongly removed > > sys/net/if_gif.h #define\tNAME > > sys/net/if_gre.c is there a reason to rename the local variables? ip_id is a global, and if_gre reuses the same name as local, hence renaming to gre_ip_id was done to reduce ambiguity in face of automated variable renaming scripts. Not sure about ip_tos -> gre_ip_tos to be honest... > sys/net/if_loop.c I cannot see a difference for vnet_loif_iattach > w/ or w/o the #ifdef. Should the outer one go? Hmm the difference is clearly visible to me, are we looking at the same code chunk? We could probably resolve IS_DEFAULT_VNET() to always true for non-vimage kernels though... +static int vnet_loif_iattach(unused) + const void *unused; +{ + INIT_VNET_NET(curvnet); + + LIST_INIT(&V_lo_list); +#ifdef VIMAGE + if (IS_DEFAULT_VNET(curvnet)) + if_clone_attach(&lo_cloner); + else + lo_cloner.ifc_attach(&lo_cloner); +#else + if_clone_attach(&lo_cloner); +#endif + return 0; +} > - " - is there a need to move the loif check up in > lo_clone_destroy? - " - junk @@ -190,7 +266,7 @@ use 4 spaces Hmm: static void lo_clone_destroy(struct ifnet *ifp) { struct lo_softc *sc; +#ifdef INVARIANTS + INIT_VNET_NET(ifp->if_vnet); +#endif sc = ifp->if_softc; /* XXX: destroying lo0 will lead to panics. */ KASSERT(V_loif != ifp, ("%s: destroying lo0", __func__)); + mtx_lock(&lo_mtx); + LIST_REMOVE(sc, sc_next); + mtx_unlock(&lo_mtx); bpfdetach(ifp); if_detach(ifp); if_free(ifp); What excatly do you see as problematic there? > sys/net/if_mib.c SYSCTL_V_INT fix ws > > sys/net/if_var.h do we need to move if_index? Yes each vnet should have a private if_index if that was the question? > sys/net/route.c static uma_zone_t rtzone; has an uneeded ws change > - " - rtable_init() ws wrong > - " - is that realted to more MRT changes or why are functions > split and shuffled? route_init() is called only once at boot time, and rtable_init() on each vnet instantiation. In nooptions VIMAGE configs route_init() hence directly calls rtable_init(). > - " - there were and still are more ws problems around > V_rt_tables - " - return 0; ws problem > - " - rtable_idetach() ws problem and more and the return > > sys/net/rtsock.c rnh =\n ... whitespace next line > > sys/net/vnet.h XXX RCS tag goes here do so > - " - struct vnet_net has ws issues with the _ether_ipfw line > - " - #define\tNAME > > > > I am running out of battery, so I am going to continue with the > next ~20%+- in sys/net80211/**, l 6556 tomorrow. > > > General: values in return statements should be enclosed in > parentheses. > > General: function declarations K&R vs. ANSI vs ... > > General: you are adding 92 lines with XXX, 18 say "locking", 2 say > WRONG, 10 say RCS, (other), ... can we get (most of) them fixed > before committing? (fixed, not removed) OK thanks for looking at the code, will do a sweeping round... Cheers, Marko