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
