pcn-helloworld: make code more clear

The logic for knowing if a port is free or not in the ports_map have been
removed, there is a new update_ports_map() function that updates the map
each time a port is added or removed.

Also some small changes to the datapath workflow are done.

Signed-off-by: Mauricio Vasquez B <mauriciovasquezbernal@gmail.com>

Author: mauriciovasquezbernal

src/services/pcn-helloworld/src/Helloworld.cpp

@@ -30,11 +30,8 @@ Helloworld::Helloworld(const std::string name, const HelloworldJsonObject &conf)
   logger()->info("Creating Helloworld instance");
   setAction(conf.getAction());
 
-  // set an initial state before doing any change to the configuration
-  // UINT16_MAX means that the port is not connected
-  auto ports_map = get_array_table<uint16_t>("ports_map");
-  ports_map.set(0, UINT16_MAX);
-  ports_map.set(1, UINT16_MAX);
+  // initialize ports map (at this point there are not ports)
+  update_ports_map();
 
   addPortsList(conf.getPorts());
 }
@@ -64,41 +61,30 @@ void Helloworld::addPorts(const std::string &name,
     throw std::runtime_error("maximum number of ports reached");
   }
 
-  auto p = add_port<PortsJsonObject>(name, conf);
+  add_port<PortsJsonObject>(name, conf);
   logger()->info("port {0} was connected", name);
-
-  auto ports_table = get_array_table<uint16_t>("ports_map");
-
-  uint32_t port_map_index;
-  try {
-    // Look for first free entry to save the port id
-    if (ports_table.get(0x0) == UINT16_MAX) {
-      port_map_index = 0x0;
-    } else if (ports_table.get(0x1) == UINT16_MAX) {
-      port_map_index = 0x1;
-    }
-  } catch (std::exception &e) {
-    logger()->error("port {0} does not exist", name);
-    // TODO: should Cube::remove_port be called?
-    throw std::runtime_error("Port does not exist");
-  }
-
-  ports_table.set(port_map_index, p->index());
+  update_ports_map();
 }
 
 void Helloworld::delPorts(const std::string &name) {
-  int index = get_port(name)->index();
+  remove_port(name);
+  logger()->info("port {0} was removed", name);
+  update_ports_map();
+}
 
+void Helloworld::update_ports_map() {
   auto ports_table = get_array_table<uint16_t>("ports_map");
+  auto ports = get_ports();
+  uint32_t i = 0;
 
-  uint32_t port_map_index;
-  if (ports_table.get(0x0) == index) {
-    port_map_index = 0x0;
-  } else if (ports_table.get(0x1) == index) {
-    port_map_index = 0x1;
+  for (auto &port: ports) {
+    ports_table.set(i, port->index());
+    i++;
+  }
+
+  // mark other ports as empty (UINT16_MAX means empty)
+  while (i < 2) {
+    ports_table.set(i, UINT16_MAX);
+    i++;
   }
-  // mark entry as free
-  ports_table.set(port_map_index, UINT16_MAX);
-  remove_port(name);
-  logger()->info("port {0} was removed", name);
 }

src/services/pcn-helloworld/src/Helloworld.h

@@ -41,4 +41,7 @@ class Helloworld : public HelloworldBase {
   /// </summary>
   HelloworldActionEnum getAction() override;
   void setAction(const HelloworldActionEnum &value) override;
+ private:
+  // saves the indexes in the ports maps used when action is forward
+  void update_ports_map();
 };

src/services/pcn-helloworld/src/Helloworld_dp_ingress.c

@@ -23,6 +23,8 @@
 #include <bcc/helpers.h>
 #include <bcc/proto.h>
 
+const uint16_t UINT16_MAX = 0xffff;
+
 enum {
   SLOWPATH_REASON = 1,
 };
@@ -56,24 +58,15 @@ BPF_ARRAY(ports_map, uint16_t, 2);
 static int handle_rx(struct CTXTYPE *ctx, struct pkt_metadata *md) {
   pcn_log(ctx, LOG_DEBUG, "Receiving packet from port %d", md->in_port);
   pcn_pkt_log(ctx, LOG_DEBUG);
+
   unsigned int zero = 0;
   unsigned int one = 1;
+
   uint8_t *action = action_map.lookup(&zero);
   if (!action) {
     return RX_DROP;
   }
 
-  // Get ports ids
-  uint16_t *p1 = ports_map.lookup(&zero);
-  if (!p1) {
-    return RX_DROP;
-  }
-
-  uint16_t *p2 = ports_map.lookup(&one);
-  if (!p2) {
-    return RX_DROP;
-  }
-
   // what action should be performed in the packet?
   switch (*action) {
   case DROP:
@@ -82,17 +75,24 @@ static int handle_rx(struct CTXTYPE *ctx, struct pkt_metadata *md) {
   case SLOWPATH:
     pcn_log(ctx, LOG_DEBUG, "Sending packet to slow path");
     return pcn_pkt_controller(ctx, md, SLOWPATH_REASON);
-  case FORWARD:
-    pcn_log(ctx, LOG_DEBUG, "Forwarding packet");
-    if (md->in_port == *p1)
-      return pcn_pkt_redirect(ctx, md, *p2);
-    else if (md->in_port == *p2)
-      return pcn_pkt_redirect(ctx, md, *p1);
-    else {
-      pcn_log(ctx, LOG_ERR, "bad in_port: %d", md->in_port);
+  case FORWARD: ;
+    // Get ports ids
+    uint16_t *p1 = ports_map.lookup(&zero);
+    if (!p1 || *p1 == UINT16_MAX) {
+      return RX_DROP;
+    }
+
+    uint16_t *p2 = ports_map.lookup(&one);
+    if (!p2 || *p2 == UINT16_MAX) {
       return RX_DROP;
     }
+
+    pcn_log(ctx, LOG_DEBUG, "Forwarding packet");
+
+    uint16_t outport = md->in_port == *p1 ? *p2 : *p1;
+    return pcn_pkt_redirect(ctx, md, outport);
   default:
+    // if control plane is well implemented this will never happen
     pcn_log(ctx, LOG_ERR, "bad action %d", *action);
     return RX_DROP;
   }