kinda headsup..
Julian Elischer
julian at elischer.org
Wed Jun 11 22:57:18 UTC 2008
Marko Zec wrote:
> 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.
for compile of LINT some more were needed..
I have checked the output of that script and all fixups into:
//repos/projects/vimage-commit2/...
LINT compiles in that tree. It's probably what will be committed for
the first step.
>
>> [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.
hmm you seem to be responding to it. so I don't see how you missed it..
>>
>> * I am missing the developers and users documentation in the
>> schedule.
Docs are on the way.
>
> 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.
fixing all the init routines is a task that will take time and
it will be something people will be fixing long after this commit is
done. It is not however something to stop this work. If you want to
not use vimage, things are as before.
>
>> - " - 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 netgraph 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...
the vimage branch is the 'head of development branch.
From that we are extracting actual commit branches...
the first commit will come from teh branch I mentioned at the top I think.
the second commit will probably come from that same branch after some
more changes have been moved to it.. At least that is how I'm
planning it.
Your comments are all however, valid..
>
>> 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
> _______________________________________________
> freebsd-virtualization at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-virtualization
> To unsubscribe, send any mail to "freebsd-virtualization-unsubscribe at freebsd.org"
More information about the freebsd-virtualization
mailing list