kinda headsup..

Marko Zec zec at
Wed Jun 11 14:56:43 UTC 2008

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 

> [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 

Developers doc: yes I know it's a showstopper, it's in a pipeline...

> > diffs can be found at:
> > 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


> sys/kern/kern_xxx.c the printf looks like debugging?


> 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 

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 

> 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);
+       if_clone_attach(&lo_cloner);
+       return 0;

>    - " -  is there a need to move the loif check up in
> lo_clone_destroy? - " - junk @@ -190,7 +266,7 @@ use 4 spaces


 static void
 lo_clone_destroy(struct ifnet *ifp)
        struct lo_softc *sc;
+       INIT_VNET_NET(ifp->if_vnet);

        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);

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...



More information about the freebsd-virtualization mailing list