[ovs-dev] Possible debug improvement in datapath/actions.c (do_output)

[Sabyasachi Sengupta]

Looking at do_output, it appears that we silently drop a packet if a bad 
out_port was specified because vport will be NULL from ovs_vport_rcu and we 
just free the skb and return from thereon. Instead, an ideal behavior should 
be to log this in user space via log_execute_message and also perhaps into 
dmesg as a warning.

In 2.3, we had do_output return an error, but it was never propagated to 
user space through do_execute_actions, but seems like fe90efd9d converted 
do_output to a 'void' function. I'm thinking if we should bring back return 
value of do_output and then have do_execute_actions return it to userspace.

Please suggest..

A patch on top of 2.3 should look like this:

diff --git a/datapath/actions.c b/datapath/actions.c
index ddacbc6..41b25aa 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -402,6 +402,7 @@ static int do_output(struct datapath *dp, struct sk_buff *skb, int out_port)
         vport = ovs_vport_rcu(dp, out_port);
         if (unlikely(!vport)) {
                 kfree_skb(skb);
+               pr_warn("invalid port number specified: %d\n", out_port);
                 return -ENODEV;
         }

@@ -584,13 +585,14 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
         int prev_port = -1;
         const struct nlattr *a;
         int rem;
+       int err;

         for (a = attr, rem = len; rem > 0;
              a = nla_next(a, &rem)) {
                 int err = 0;

                 if (prev_port != -1) {
-                       do_output(dp, skb_clone(skb, GFP_ATOMIC), prev_port);
+                       err = do_output(dp, skb_clone(skb, GFP_ATOMIC), prev_port);
                         prev_port = -1;
                 }

@@ -651,11 +653,11 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
         }

         if (prev_port != -1)
-               do_output(dp, skb, prev_port);
+               err = do_output(dp, skb, prev_port);
         else
                 consume_skb(skb);

-       return 0;
+       return err;
  }

  /* We limit the number of times that we pass into execute_actions()