s/HAVE_NETNS/HAVE_SETNS in my post. I fat fingered the #define name :(. I still think using the #define setup in the confgure.ac( and then config.h ) would be better than VRF_USE_NETNS as well as GNU_LINUX.
donald On Wed, Jun 3, 2015 at 4:19 AM, Nicolas Dichtel <[email protected]> wrote: > Le 02/06/2015 21:52, Donald Sharp a écrit : > >> Some minor comments inline >> >> On Tue, Jun 2, 2015 at 11:57 AM, Nicolas Dichtel < >> [email protected]> >> wrote: >> >> [snip] > >> 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? >> > Right for FreeBSD, but '#ifdef GNU_LINUX' is probably better. > > >> >> #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? >> > HAVE_NETNS does not exist for now, only HAVE_SETNS is defined. > > >> >> >> + >>> +#define VRF_USE_NETNS 1 >>> >>> >> This seems like an extra unnecessary #define? HAVE_NETNS already >> encompasses this? >> > There is no HAVE_NETNS, only a HAVE_SETNS. > HAVE_SETNS is set when the syscall setns() is provided by the toolchain. > But > you can have an old toolchain with a recent linux kernel. In that case, > HAVE_SETNS won't be defined but netns will be supported by your kernel. > > >> >> + >>> +#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? >> > Yes, we don't have setns(), this just means that the toolchain does not > provide > that syscall, it does not mean that your kernel doesn't support it. > __NR_setns is provided by the linux kernel headers, hence, if this value is > defined, it means that your kernel supports netns and you can do the > syscall > "manually". > > You can have a look at iproute2, there is a similar mechanism: > > https://git.kernel.org/cgit/linux/kernel/git/shemminger/iproute2.git/tree/include/namespace.h#n34 > > [snip] > > @@ -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 >> > Answered above. > > >> >> + 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 >> > Good point, I will do that. > > > >> >> >> + { >>> + 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 >> > Right! > > [snip] > > +#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. >> > Ok, I will rework this part. > > > Thank you for the review, > Nicolas >
_______________________________________________ Quagga-dev mailing list [email protected] https://lists.quagga.net/mailman/listinfo/quagga-dev
