Ensure the plugin works well with namespaces

I've made a few cosmetic adjustments:
- introduce debug, info, notice, warn and err loggers
- remove redundant logging, and set correct (conservative) log levels
- turn the sync-state error into a warning

And a little debt paydown:
- refactor sync-state into its own function, to be called instead of
  all the spot fixes elsewhere. It's going to be the case that
  sync-state is "the reconsiliation function".
- Fix a bug in lip->lip_namespace copy: vec_dup() there doesn't seem
  to work for me, use strdup() instead and make a mental note to
  reviist.

The plugin now works with 'lcpng default netns dataplane' in its
startup.conf; and with 'lcp default netns dataplane' as its first
command. A few of these fixes should go upstream, too, which I'll
do next.

lcpng_if_sync.c

@@ -36,28 +36,23 @@
 #include <linux/netlink.h>
 #include <linux/rtnetlink.h>
 
-/* walk function to copy forward all sw interface link state flags
+/* helper function to copy forward all sw interface link state flags
  * MTU, and IP addresses into their counterpart LIP interface.
  *
  * This is called upon MTU changes and state changes.
  */
-static walk_rc_t
+void
-lcp_itf_pair_walk_sync_state_cb (index_t lipi, void *ctx)
+lcp_itf_pair_sync_state (lcp_itf_pair_t *lip)
 {
-  lcp_itf_pair_t *lip;
   vnet_sw_interface_t *sw;
   vnet_sw_interface_t *sup_sw;
   int curr_ns_fd = -1;
   int vif_ns_fd = -1;
 
-  lip = lcp_itf_pair_get (lipi);
-  if (!lip)
-    return WALK_CONTINUE;
-
   sw =
     vnet_get_sw_interface_or_null (vnet_get_main (), lip->lip_phy_sw_if_index);
   if (!sw)
-    return WALK_CONTINUE;
+    return;
   sup_sw =
     vnet_get_sw_interface_or_null (vnet_get_main (), sw->sup_sw_if_index);
 
@@ -69,7 +64,7 @@ lcp_itf_pair_walk_sync_state_cb (index_t lipi, void *ctx)
 	clib_setns (vif_ns_fd);
     }
 
-  LCP_ITF_PAIR_DBG ("walk_sync_state: %U flags %u mtu %u sup-mtu %u",
+  LCP_ITF_PAIR_INFO ("sync_state: %U flags %u mtu %u sup-mtu %u",
 		     format_lcp_itf_pair, lip, sw->flags, sw->mtu[VNET_MTU_L3],
 		     sup_sw->mtu[VNET_MTU_L3]);
   lcp_itf_set_link_state (lip, (sw->flags & VNET_SW_INTERFACE_FLAG_ADMIN_UP));
@@ -79,7 +74,7 @@ lcp_itf_pair_walk_sync_state_cb (index_t lipi, void *ctx)
    */
   if (sup_sw->mtu[VNET_MTU_L3] < sw->mtu[VNET_MTU_L3])
     {
-      LCP_ITF_PAIR_ERR ("walk_sync_state: %U flags %u mtu %u sup-mtu %u: "
+      LCP_ITF_PAIR_WARN ("sync_state: %U flags %u mtu %u sup-mtu %u: "
 			"clamping to sup-mtu to satisfy netlink",
 			format_lcp_itf_pair, lip, sw->flags,
 			sw->mtu[VNET_MTU_L3], sup_sw->mtu[VNET_MTU_L3]);
@@ -89,13 +84,9 @@ lcp_itf_pair_walk_sync_state_cb (index_t lipi, void *ctx)
     }
 
   /* Linux will remove IPv6 addresses on children when the master state
-   * goes down, so we ensure all IPv4/IPv6 addresses are set when the phy
+   * goes down, so we ensure all IPv4/IPv6 addresses are synced.
-   * comes back up.
    */
-  if (sw->flags & VNET_SW_INTERFACE_FLAG_ADMIN_UP)
-    {
   lcp_itf_set_interface_addr (lip);
-    }
 
   if (vif_ns_fd != -1)
     close (vif_ns_fd);
@@ -106,6 +97,18 @@ lcp_itf_pair_walk_sync_state_cb (index_t lipi, void *ctx)
       close (curr_ns_fd);
     }
 
+  return;
+}
+
+static walk_rc_t
+lcp_itf_pair_walk_sync_state_cb (index_t lipi, void *ctx)
+{
+  lcp_itf_pair_t *lip;
+  lip = lcp_itf_pair_get (lipi);
+  if (!lip)
+    return WALK_CONTINUE;
+
+  lcp_itf_pair_sync_state (lip);
   return WALK_CONTINUE;
 }
 
@@ -171,7 +174,7 @@ static clib_error_t *
 lcp_itf_mtu_change (vnet_main_t *vnm, u32 sw_if_index, u32 flags)
 {
   const lcp_itf_pair_t *lip;
-  vnet_sw_interface_t *si;
+  vnet_sw_interface_t *sw;
   int curr_ns_fd = -1;
   int vif_ns_fd = -1;
 
@@ -183,8 +186,8 @@ lcp_itf_mtu_change (vnet_main_t *vnm, u32 sw_if_index, u32 flags)
   if (!lip)
     return NULL;
 
-  si = vnet_get_sw_interface_or_null (vnm, sw_if_index);
+  sw = vnet_get_sw_interface_or_null (vnm, sw_if_index);
-  if (!si)
+  if (!sw)
     return NULL;
 
   if (lip->lip_namespace)
@@ -196,8 +199,8 @@ lcp_itf_mtu_change (vnet_main_t *vnm, u32 sw_if_index, u32 flags)
     }
 
   LCP_ITF_PAIR_INFO ("mtu_change: %U mtu %u", format_lcp_itf_pair, lip,
-		     si->mtu[VNET_MTU_L3]);
+		     sw->mtu[VNET_MTU_L3]);
-  vnet_netlink_set_link_mtu (lip->lip_vif_index, si->mtu[VNET_MTU_L3]);
+  vnet_netlink_set_link_mtu (lip->lip_vif_index, sw->mtu[VNET_MTU_L3]);
   if (vif_ns_fd != -1)
     close (vif_ns_fd);
 

lcpng_interface.c

@@ -129,8 +129,7 @@ lcp_itf_pair_show (u32 phy_sw_if_index)
 
   vm = vlib_get_main ();
   ns = lcp_get_default_ns();
-  vlib_cli_output (vm, "lcpng netns '%s'\n",
+  vlib_cli_output (vm, "lcp netns %s\n", ns ? (char *) ns : "<unset>");
-		   ns ? (char *) ns : "<unset>");
 
   if (phy_sw_if_index == ~0)
     {
@@ -242,7 +241,7 @@ lcp_itf_pair_add (u32 host_sw_if_index, u32 phy_sw_if_index, u8 *host_name,
 
   lipi = lcp_itf_pair_find_by_phy (phy_sw_if_index);
 
-  LCP_ITF_PAIR_INFO ("add: host:%U phy:%U, host_if:%v vif:%d ns:%v",
+  LCP_ITF_PAIR_INFO ("pair_add: host:%U phy:%U, host_if:%v vif:%d ns:%s",
 		     format_vnet_sw_if_index_name, vnet_get_main (),
 		     host_sw_if_index, format_vnet_sw_if_index_name,
 		     vnet_get_main (), phy_sw_if_index, host_name, host_index,
@@ -269,7 +268,11 @@ lcp_itf_pair_add (u32 host_sw_if_index, u32 phy_sw_if_index, u8 *host_name,
   lip->lip_host_name = vec_dup (host_name);
   lip->lip_host_type = host_type;
   lip->lip_vif_index = host_index;
-  lip->lip_namespace = vec_dup (ns);
+
+  // TODO(pim) - understand why vec_dup(ns) returns 'nil' here
+  lip->lip_namespace = 0;
+  if (ns && ns[0] != 0)
+    lip->lip_namespace = (u8 *) strdup ((const char *) ns);
 
   if (lip->lip_host_sw_if_index == ~0)
     return 0;
@@ -629,6 +632,9 @@ lcp_itf_set_interface_addr (const lcp_itf_pair_t *lip)
   int vif_ns_fd = -1;
   int curr_ns_fd = -1;
 
+  if (!lip)
+    return;
+
   if (lip->lip_namespace)
     {
       curr_ns_fd = clib_netns_open (NULL /* self */);
@@ -685,9 +691,11 @@ lcp_itf_pair_find_walk (vnet_main_t *vnm, u32 sw_if_index, void *arg)
 
   sw = vnet_get_sw_interface_or_null (vnm, sw_if_index);
   if (sw && (sw->sub.eth.inner_vlan_id == 0) && (sw->sub.eth.outer_vlan_id == match->vlan) && (sw->sub.eth.flags.dot1ad == match->dot1ad)) {
-    LCP_ITF_PAIR_DBG ("pair_create: found match %U outer %d dot1ad %d inner-dot1q %d",
+      LCP_ITF_PAIR_DBG (
+	"find_walk: found match %U outer %d dot1ad %d inner-dot1q %d",
 	format_vnet_sw_if_index_name, vnet_get_main (), sw_if_index,
-		      sw->sub.eth.outer_vlan_id, sw->sub.eth.flags.dot1ad, sw->sub.eth.inner_vlan_id);
+	sw->sub.eth.outer_vlan_id, sw->sub.eth.flags.dot1ad,
+	sw->sub.eth.inner_vlan_id);
       match->matched_sw_if_index = sw_if_index;
       return WALK_STOP;
   }
@@ -752,7 +760,7 @@ lcp_itf_pair_create (u32 phy_sw_if_index, u8 *host_if_name,
   u32 vif_index = 0, host_sw_if_index;
   const vnet_sw_interface_t *sw;
   const vnet_hw_interface_t *hw;
-  const lcp_itf_pair_t *lip;
+  lcp_itf_pair_t *lip;
 
   if (!vnet_sw_if_index_is_api_valid (phy_sw_if_index)) {
     LCP_ITF_PAIR_ERR ("pair_create: invalid phy index %u", phy_sw_if_index);
@@ -936,7 +944,9 @@ lcp_itf_pair_create (u32 phy_sw_if_index, u8 *host_if_name,
 	}
 
       if (ns && ns[0] != 0)
+	{
 	  args.host_namespace = ns;
+	}
 
       vm = vlib_get_main ();
       tap_create_if (vm, &args);
@@ -983,17 +993,13 @@ lcp_itf_pair_create (u32 phy_sw_if_index, u8 *host_if_name,
 
   if (!vif_index)
     {
-      LCP_ITF_PAIR_INFO ("failed pair add (no vif index): {%U, %U, %s}",
+      LCP_ITF_PAIR_ERR (
-			 format_vnet_sw_if_index_name, vnet_get_main (),
+	"pair_create: failed pair add (no vif index): {%U, %U, %s}",
-			 phy_sw_if_index, format_vnet_sw_if_index_name,
-			 vnet_get_main (), host_sw_if_index, host_if_name);
-      return -1;
-    }
-
-  LCP_ITF_PAIR_INFO ("pair create: {%U, %U, %s}",
 	format_vnet_sw_if_index_name, vnet_get_main (), phy_sw_if_index,
 	format_vnet_sw_if_index_name, vnet_get_main (), host_sw_if_index,
 	host_if_name);
+      return -1;
+    }
 
   lcp_itf_pair_add (host_sw_if_index, phy_sw_if_index, host_if_name, vif_index,
 		    host_if_type, ns);
@@ -1005,15 +1011,7 @@ lcp_itf_pair_create (u32 phy_sw_if_index, u8 *host_if_name,
    */
   vnet_sw_interface_admin_up (vnm, host_sw_if_index);
   lip = lcp_itf_pair_get (lcp_itf_pair_find_by_vif(vif_index));
-  LCP_ITF_PAIR_INFO ("pair_create: copy link state %u from %U to %U",
+  lcp_itf_pair_sync_state (lip);
-		  sw->flags,
-		  format_vnet_sw_if_index_name, vnet_get_main (), sw->sw_if_index,
-		  format_lcp_itf_pair, lip);
-  lcp_itf_set_link_state (lip, sw->flags & VNET_SW_INTERFACE_FLAG_ADMIN_UP);
-
-  /* Copy L3 addresses from VPP into the host side, if any.
-   */
-  lcp_itf_set_interface_addr (lip);
 
   if (host_sw_if_indexp)
     *host_sw_if_indexp = host_sw_if_index;

lcpng_interface.h

@@ -26,11 +26,17 @@
 extern vlib_log_class_t lcp_itf_pair_logger;
 
 #define LCP_ITF_PAIR_DBG(...)                                                 \
-  vlib_log_notice (lcp_itf_pair_logger, __VA_ARGS__);
+  vlib_log_debug (lcp_itf_pair_logger, __VA_ARGS__);
 
 #define LCP_ITF_PAIR_INFO(...)                                                \
+  vlib_log_info (lcp_itf_pair_logger, __VA_ARGS__);
+
+#define LCP_ITF_PAIR_NOTICE(...)                                                \
   vlib_log_notice (lcp_itf_pair_logger, __VA_ARGS__);
 
+#define LCP_ITF_PAIR_WARN(...)                                                \
+  vlib_log_warn (lcp_itf_pair_logger, __VA_ARGS__);
+
 #define LCP_ITF_PAIR_ERR(...)                                                \
   vlib_log_err (lcp_itf_pair_logger, __VA_ARGS__);
 
@@ -187,6 +193,14 @@ void lcp_itf_ip6_add_del_interface_addr (ip6_main_t *im, uword opaque,
 					 u32 address_length,
 					 u32 if_address_index, u32 is_del);
 
+/* Sync all state from VPP to Linux device
+ *
+ * Note: in some circumstances, this syncer will (have to) make changes to
+ * the VPP interface, for example if its MTU is greater than its parent.
+ * See the function for rationale.
+ */
+void lcp_itf_pair_sync_state (lcp_itf_pair_t *lip);
+
 /*
  * fd.io coding-style-patch-verification: ON
  *