Some minor comments inline On Tue, Jun 2, 2015 at 11:57 AM, Nicolas Dichtel <nicolas.dich...@6wind.com> wrote:
> From: Feng Lu <lu.f...@6wind.com> > > Linux supports netns from version 3.x. We realize VRFs with > linux netns by default. The main job is to associate a VRF > with a netns. Currently this is done by the configuration: > > [no] vrf N netns <netns-name> > > This command is also available in vtysh and goes to only > zebra, because presently only zebra supports multiple VRF. > > A file descriptor is added to "struct vrf". This is for the > associated netns file. Once the command "vrf N netns NAME" > is executed, the specified file is opened and the file > descriptor is stored in the VRF N. In this way the > association is formed. > > In vrf_socket(), we first switch to the specified VRF by > using the stored file descriptor, and then can allocate > a socket which is working in the associated netns. > > Signed-off-by: Feng Lu <lu.f...@6wind.com> > Reviewed-by: Alain Ritoux <alain.rit...@6wind.com> > Signed-off-by: Nicolas Dichtel <nicolas.dich...@6wind.com> > --- > configure.ac | 2 + > lib/command.h | 1 + > lib/vrf.c | 238 > +++++++++++++++++++++++++++++++++++++++++++++++++++- > vtysh/Makefile.am | 1 + > vtysh/extract.pl.in | 3 + > 5 files changed, 241 insertions(+), 4 deletions(-) > > diff --git a/configure.ac b/configure.ac > index f68d86fc4f07..5f273ae48a9d 100755 > --- a/configure.ac > +++ b/configure.ac > @@ -801,6 +801,8 @@ AC_CHECK_FUNCS(setproctitle, , > ] > ) > > +AC_CHECK_FUNCS(setns, AC_DEFINE(HAVE_SETNS,, Have setns)) > + > dnl ------------------------------------ > dnl Determine routing get and set method > dnl ------------------------------------ > diff --git a/lib/command.h b/lib/command.h > index a36a524a048f..bb0122fa4d5f 100644 > --- a/lib/command.h > +++ b/lib/command.h > @@ -68,6 +68,7 @@ enum node_type > AUTH_ENABLE_NODE, /* Authentication mode for change enable. > */ > ENABLE_NODE, /* Enable node. */ > CONFIG_NODE, /* Config node. Default mode of config > file. */ > + VRF_NODE, /* VRF node. */ > SERVICE_NODE, /* Service node. */ > DEBUG_NODE, /* Debug node. */ > AAA_NODE, /* AAA node. */ > diff --git a/lib/vrf.c b/lib/vrf.c > index 683026e5c069..c3e870ef8960 100644 > --- a/lib/vrf.c > +++ b/lib/vrf.c > @@ -22,14 +22,48 @@ > > #include <zebra.h> > > +#undef _GNU_SOURCE > +#define _GNU_SOURCE > + > +#include <sched.h> > + > Shouldn't this bit be wrappered by #if defined(HAVE_NETNS)? Why would freebsd want to include this bit? > #include "if.h" > #include "vrf.h" > #include "prefix.h" > #include "table.h" > #include "log.h" > #include "memory.h" > +#include "command.h" > +#include "vty.h" > + > +#ifdef GNU_LINUX > Shouldn't this be: #if defined(HAVE_NETS) instead of #ifdef GNU_LINUX? > + > +#define VRF_USE_NETNS 1 > This seems like an extra unnecessary #define? HAVE_NETNS already encompasses this? > + > +#ifndef CLONE_NEWNET > +#define CLONE_NEWNET 0x40000000 /* New network namespace (lo, device, > names sockets, etc) */ > +#endif > + > +#ifndef HAVE_SETNS > +static inline int setns(int fd, int nstype) > +{ > +#ifdef __NR_setns > + return syscall(__NR_setns, fd, nstype); > Can you help me to understand this #ifdef __NR_SETNS? I'm lost on what this is supposed to do. We've already established we don't have setns? > +#else > + errno = ENOSYS; > + return -1; > +#endif > +} > +#endif /* HAVE_SETNS */ > + > +#endif /* CLONE_NEWNET */ > > +#ifdef VRF_USE_NETNS > +#define VRF_RUN_DIR "/var/run/netns" > +#define VRF_DEFAULT_NAME "/proc/self/ns/net" > +#else > #define VRF_DEFAULT_NAME "Default-IP-Routing-Table" > +#endif > > struct vrf > { > @@ -37,6 +71,8 @@ struct vrf > vrf_id_t vrf_id; > /* Name */ > char *name; > + /* File descriptor */ > + int fd; > > /* Master list of interfaces belonging to this VRF */ > struct list *iflist; > @@ -90,6 +126,7 @@ vrf_get (vrf_id_t vrf_id) > > vrf = XCALLOC (MTYPE_VRF, sizeof (struct vrf)); > vrf->vrf_id = vrf_id; > + vrf->fd = -1; > rn->info = vrf; > > /* Initialize interfaces. */ > @@ -149,7 +186,11 @@ vrf_lookup (vrf_id_t vrf_id) > static int > vrf_is_enabled (struct vrf *vrf) > { > +#ifdef VRF_USE_NETNS > As above HAVE_NETNS -vs- VRF_USE_NETNS > + return vrf && vrf->fd >= 0; +#else > return vrf && vrf->vrf_id == VRF_DEFAULT; > +#endif > } > > /* > @@ -162,7 +203,30 @@ vrf_is_enabled (struct vrf *vrf) > static int > vrf_enable (struct vrf *vrf) > { > - /* Till now, only the default VRF can be enabled. */ > +#ifdef VRF_USE_NETNS > + > + if (!vrf_is_enabled (vrf)) > I do not believe this if statement needs to be part of the #ifdef. in vrf_is_enabled(), the code already correctly does the vrf check. As such I would move the #ifdef to inside the if statement and loose the #else condition > + { > + vrf->fd = open (vrf->name, O_RDONLY); > + > + if (!vrf_is_enabled (vrf)) > + { > + zlog_err ("Can not enable VRF %u: %s!", > + vrf->vrf_id, safe_strerror (errno)); > + return 0; > + } > + > + zlog_info ("VRF %u is associated with NETNS %s.", > + vrf->vrf_id, vrf->name); > + > + if (vrf_master.vrf_enable_hook) > + (*vrf_master.vrf_enable_hook) (vrf->vrf_id, &vrf->info); > + } > + > + return 1; > + > +#else > + > if (vrf->vrf_id == VRF_DEFAULT) > { > zlog_info ("VRF %u is enabled.", vrf->vrf_id); > @@ -174,6 +238,8 @@ vrf_enable (struct vrf *vrf) > } > > return 0; > + > +#endif > } > > /* > @@ -188,10 +254,13 @@ vrf_disable (struct vrf *vrf) > { > zlog_info ("VRF %u is to be disabled.", vrf->vrf_id); > > - /* Till now, nothing to be done for the default VRF. */ > - > if (vrf_master.vrf_disable_hook) > (*vrf_master.vrf_disable_hook) (vrf->vrf_id, &vrf->info); > + > +#ifdef VRF_USE_NETNS > + close (vrf->fd); > + vrf->fd = -1; > +#endif > } > } > > @@ -429,6 +498,145 @@ vrf_bitmap_check (vrf_bitmap_t bmap, vrf_id_t vrf_id) > VRF_BITMAP_FLAG (offset)) ? 1 : 0; > } > > +/* > + * VRF realization with NETNS > + */ > +#ifdef VRF_USE_NETNS > + > +static char * > +vrf_netns_pathname (struct vty *vty, const char *name) > +{ > + static char pathname[PATH_MAX]; > + char *result; > + > + if (name[0] == '/') /* absolute pathname */ > + result = realpath (name, pathname); > + else /* relevant pathname */ > relative pathname > + { > + char tmp_name[PATH_MAX]; > + snprintf (tmp_name, PATH_MAX, "%s/%s", VRF_RUN_DIR, name); > + result = realpath (tmp_name, pathname); > + } > + > + if (! result) > + { > + vty_out (vty, "Invalid pathname: %s%s", safe_strerror (errno), > + VTY_NEWLINE); > + return NULL; > + } > + return pathname; > +} > + > +DEFUN (vrf_netns, > + vrf_netns_cmd, > + "vrf <1-65535> netns NAME", > + "Enable a VRF\n" > + "Specify the VRF identifier\n" > + "Associate with a NETNS\n" > + "The file name in " VRF_RUN_DIR ", or a full pathname\n") > +{ > + vrf_id_t vrf_id = VRF_DEFAULT; > + struct vrf *vrf = NULL; > + char *pathname = vrf_netns_pathname (vty, argv[1]); > + > + if (!pathname) > + return CMD_WARNING; > + > + VTY_GET_INTEGER ("VRF ID", vrf_id, argv[0]); > + vrf = vrf_get (vrf_id); > + > + if (vrf->name && strcmp (vrf->name, pathname) != 0) > + { > + vty_out (vty, "VRF %u is already configured with NETNS %s%s", > + vrf->vrf_id, vrf->name, VTY_NEWLINE); > + return CMD_WARNING; > + } > + > + if (!vrf->name) > + vrf->name = XSTRDUP (MTYPE_VRF_NAME, pathname); > + > + if (!vrf_enable (vrf)) > + { > + vty_out (vty, "Can not associate VRF %u with NETNS %s%s", > + vrf->vrf_id, vrf->name, VTY_NEWLINE); > + return CMD_WARNING; > + } > + > + return CMD_SUCCESS; > +} > + > +DEFUN (no_vrf_netns, > + no_vrf_netns_cmd, > + "no vrf <1-65535> netns NAME", > + NO_STR > + "Enable a VRF\n" > + "Specify the VRF identifier\n" > + "Associate with a NETNS\n" > + "The file name in " VRF_RUN_DIR ", or a full pathname\n") > +{ > + vrf_id_t vrf_id = VRF_DEFAULT; > + struct vrf *vrf = NULL; > + char *pathname = vrf_netns_pathname (vty, argv[1]); > + > + if (!pathname) > + return CMD_WARNING; > + > + VTY_GET_INTEGER ("VRF ID", vrf_id, argv[0]); > + vrf = vrf_lookup (vrf_id); > + > + if (!vrf) > + { > + vty_out (vty, "VRF %u is not found%s", vrf_id, VTY_NEWLINE); > + return CMD_SUCCESS; > + } > + > + if (vrf->name && strcmp (vrf->name, pathname) != 0) > + { > + vty_out (vty, "Incorrect NETNS file name%s", VTY_NEWLINE); > + return CMD_WARNING; > + } > + > + if (vrf_is_enabled (vrf)) > + vrf_disable (vrf); > + > + if (vrf->name) > + { > + XFREE (MTYPE_VRF_NAME, vrf->name); > + vrf->name = NULL; > + } > + > + return CMD_SUCCESS; > +} > + > +/* VRF node. */ > +static struct cmd_node vrf_node = > +{ > + VRF_NODE, > + "", /* VRF node has no interface. */ > + 1 > +}; > + > +/* VRF configuration write function. */ > +static int > +vrf_config_write (struct vty *vty) > +{ > + struct route_node *rn; > + struct vrf *vrf; > + int write = 0; > + > + for (rn = route_top (vrf_table); rn; rn = route_next (rn)) > + if ((vrf = rn->info) != NULL && > + vrf->vrf_id != VRF_DEFAULT && vrf->name) > + { > + vty_out (vty, "vrf %u netns %s%s", vrf->vrf_id, vrf->name, > VTY_NEWLINE); > + write++; > + } > + > + return write; > +} > + > +#endif > + > /* Initialize VRF module. */ > void > vrf_init (void) > @@ -455,6 +663,13 @@ vrf_init (void) > zlog_err ("vrf_init: failed to enable the default VRF!"); > exit (1); > } > + > +#ifdef VRF_USE_NETNS > + /* Install VRF commands. */ > + install_node (&vrf_node, vrf_config_write); > + install_element (CONFIG_NODE, &vrf_netns_cmd); > + install_element (CONFIG_NODE, &no_vrf_netns_cmd); > +#endif > } > > /* Terminate VRF module. */ > @@ -476,19 +691,34 @@ vrf_terminate (void) > int > vrf_socket (int domain, int type, int protocol, vrf_id_t vrf_id) > { > + struct vrf *vrf = vrf_lookup (vrf_id); > int ret = -1; > > - if (!vrf_is_enabled (vrf_lookup (vrf_id))) > + if (!vrf || !vrf_is_enabled (vrf)) > { > errno = ENOSYS; > return -1; > } > > +#ifdef VRF_USE_NETNS > Why do we need this setns call protected by VRF_USE_NETNS? We've already provided a wrapper function that does the right thing? I would prefer that this section of code just does the right thing irrelevant of what platform it is on instead of #ifdef protections. thanks! donald > + > + ret = (vrf_id != VRF_DEFAULT) ? setns (vrf->fd, CLONE_NEWNET) : 0; > + if (ret >= 0) > + { > + ret = socket (domain, type, protocol); > + if (vrf_id != VRF_DEFAULT) > + setns (vrf_lookup (VRF_DEFAULT)->fd, CLONE_NEWNET); > + } > + > +#else > + > if (vrf_id == VRF_DEFAULT) > ret = socket (domain, type, protocol); > else > errno = ENOSYS; > > +#endif > + > return ret; > } > > diff --git a/vtysh/Makefile.am b/vtysh/Makefile.am > index d1ff69b9c96a..850b505e5d62 100644 > --- a/vtysh/Makefile.am > +++ b/vtysh/Makefile.am > @@ -28,6 +28,7 @@ vtysh_cmd_FILES = $(top_srcdir)/bgpd/*.c > $(top_srcdir)/isisd/*.c \ > $(top_srcdir)/lib/keychain.c > $(top_srcdir)/lib/routemap.c \ > $(top_srcdir)/lib/filter.c $(top_srcdir)/lib/plist.c \ > $(top_srcdir)/lib/distribute.c > $(top_srcdir)/lib/if_rmap.c \ > + $(top_srcdir)/lib/vrf.c \ > $(top_srcdir)/lib/vty.c $(top_srcdir)/zebra/debug.c \ > $(top_srcdir)/zebra/interface.c \ > $(top_srcdir)/zebra/irdp_interface.c \ > diff --git a/vtysh/extract.pl.in b/vtysh/extract.pl.in > index f057e24922e1..aa90be4a19b1 100755 > --- a/vtysh/extract.pl.in > +++ b/vtysh/extract.pl.in > @@ -99,6 +99,9 @@ foreach (@ARGV) { > elsif ($file =~ /lib\/filter\.c$/) { > $protocol = "VTYSH_ALL"; > } > + elsif ($file =~ /lib\/vrf\.c$/) { > + $protocol = "VTYSH_ZEBRA"; > + } > elsif ($file =~ /lib\/plist\.c$/) { > if ($defun_array[1] =~ m/ipv6/) { > $protocol = > "VTYSH_RIPNGD|VTYSH_OSPF6D|VTYSH_BGPD|VTYSH_ZEBRA"; > -- > 2.4.2 > > > _______________________________________________ > Quagga-dev mailing list > Quagga-dev@lists.quagga.net > https://lists.quagga.net/mailman/listinfo/quagga-dev >
_______________________________________________ Quagga-dev mailing list Quagga-dev@lists.quagga.net https://lists.quagga.net/mailman/listinfo/quagga-dev