[ovs-dev] [PATCH 6/6] ofproto: Enable in-place modification for recirc actions.

[Joe Stringer]

On 28 October 2015 at 20:07, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>

Looks ok to me. I have some minor style/doc suggestions at the end of the mail.

In revalidate(), the way that 'recircs' is only initialised in some
paths, then in an overlapping set of paths passed to reval_op_init(),
seems prone to cause confusion or errors if the logic is further
modified. I think it's correct, but I wonder if there's a better way
we could compose the logic to make it more obvious (It's partly due to
the new function reval_op_init()).

Otherwise, up to you whether you'd like another pair of eyes on this code.

Acked-by: Joe Stringer <joestringer at nicira.com>


diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h
index b5cbc739fa02..135810d0b027 100644
--- a/ofproto/ofproto-dpif-rid.h
+++ b/ofproto/ofproto-dpif-rid.h
@@ -21,7 +21,6 @@
 #include <stdint.h>

 #include "cmap.h"
-#include "flow.h"
 #include "list.h"
 #include "ofp-actions.h"
 #include "ofproto-dpif-mirror.h"
@@ -206,12 +205,13 @@ struct recirc_refs {
     };
 };

-#define RECIRC_REFS_EMPTY_INITIALIZER { 0, { { 0, 0 } } }
+#define RECIRC_REFS_EMPTY_INITIALIZER ((struct recirc_refs) \
+                                       { 0, { { 0, 0 } } })
 /* Helpers to abstract the recirculation union away. */
 static inline void
 recirc_refs_init(struct recirc_refs *rr)
 {
-    memset(rr, 0, sizeof *rr);
+    *rr = RECIRC_REFS_EMPTY_INITIALIZER;
 }

 static inline void
@@ -1783,7 +1784,13 @@ should_revalidate(const struct udpif *udpif,
uint64_t packets,
  *                  fine.  Callers should change the actions to those found
  *                  in the caller supplied 'odp_actions' buffer.  The
  *                  recirculation references can be found in 'recircs' and
- *                  must be handled by the caller. */
+ *                  must be handled by the caller.
+ *
+ * If the result is UKEY_MODIFY, then references to all recirc_ids used by the
+ * new flow will be held within 'recircs' (which may be none). The caller
+ * is responsible for ensuring that 'recircs' references are eventually freed.
+ * In all other cases, 'recircs' is initialised to have no references.
+ */
 static enum reval_result
 revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
                 const struct dpif_flow_stats *stats,