ofp-actions: Make composing actions harder to screw up.
authorBen Pfaff <blp@ovn.org>
Wed, 23 Dec 2015 21:23:48 +0000 (13:23 -0800)
committerBen Pfaff <blp@ovn.org>
Tue, 5 Jan 2016 00:07:51 +0000 (16:07 -0800)
Until now, composing a fixed-length action with ofpact_put_<NAME>() failed
to append any padding required after the action.  This commit changes that
so that these calls now add padding.  This meant that the function
ofpact_pad(), which was until now required in various unintuitive places,
is no longer required, and removes it.

Variable-length actions still require calling ofpact_update_len() after
composition.  I don't see a way to avoid that.

Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Russell Bryant <russell@ovn.org>
lib/learn.c
lib/learning-switch.c
lib/ofp-actions.c
lib/ofp-actions.h
ofproto/connmgr.c
ofproto/fail-open.c
ofproto/ofproto-dpif-xlate.c
ofproto/ofproto-dpif.c

index 4b2cc97..50627ca 100644 (file)
@@ -161,7 +161,6 @@ learn_execute(const struct ofpact_learn *learn, const struct flow *flow,
             break;
         }
     }
-    ofpact_pad(ofpacts);
 
     fm->ofpacts = ofpacts->data;
     fm->ofpacts_len = ofpacts->size;
index 28f3819..172a8dc 100644 (file)
@@ -642,7 +642,6 @@ process_packet_in(struct lswitch *sw, const struct ofp_header *oh)
         enqueue->port = out_port;
         enqueue->queue = queue_id;
     }
-    ofpact_pad(&ofpacts);
 
     /* Prepare packet_out in case we need one. */
     po.buffer_id = pi.buffer_id;
index f900aa9..921295a 100644 (file)
@@ -444,7 +444,6 @@ ofpacts_pull(struct ofpbuf *ofpacts)
 {
     size_t ofs;
 
-    ofpact_pad(ofpacts);
     ofs = ofpacts->size;
     ofpbuf_pull(ofpacts, ofs);
 
@@ -4376,10 +4375,10 @@ decode_NXAST_RAW_NOTE(const struct nx_action_note *nan,
     unsigned int length;
 
     length = ntohs(nan->len) - offsetof(struct nx_action_note, note);
-    note = ofpact_put(out, OFPACT_NOTE,
-                      offsetof(struct ofpact_note, data) + length);
+    note = ofpact_put_NOTE(out);
     note->length = length;
-    memcpy(note->data, nan->note, length);
+    ofpbuf_put(out, nan->note, length);
+    ofpact_update_len(out, out->header);
 
     return 0;
 }
@@ -4930,7 +4929,6 @@ parse_CT(char *arg, struct ofpbuf *ofpacts,
             const size_t nat_offset = ofpacts_pull(ofpacts);
 
             error = parse_NAT(value, ofpacts, usable_protocols);
-            ofpact_pad(ofpacts);
             /* Update CT action pointer and length. */
             ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
             oc = ofpacts->header;
@@ -4946,7 +4944,6 @@ parse_CT(char *arg, struct ofpbuf *ofpacts,
             error = ofpacts_parse_copy(value, ofpacts, &usable_protocols2,
                                        false, OFPACT_CT);
             *usable_protocols &= usable_protocols2;
-            ofpact_pad(ofpacts);
             ofpacts->header = ofpbuf_push_uninit(ofpacts, exec_offset);
             oc = ofpacts->header;
         } else {
@@ -5622,8 +5619,6 @@ ofpacts_decode(const void *actions, size_t actions_len,
             return error;
         }
     }
-
-    ofpact_pad(ofpacts);
     return 0;
 }
 
@@ -6297,10 +6292,7 @@ ofpacts_pull_openflow_instructions(struct ofpbuf *openflow,
         struct ofpact_nest *on;
         const struct ofp_action_header *actions;
         size_t actions_len;
-        size_t start;
-
-        ofpact_pad(ofpacts);
-        start = ofpacts->size;
+        size_t start = ofpacts->size;
         ofpact_put(ofpacts, OFPACT_WRITE_ACTIONS,
                    offsetof(struct ofpact_nest, actions));
         get_actions_from_instruction(insts[OVSINST_OFPIT11_WRITE_ACTIONS],
@@ -6334,8 +6326,6 @@ ofpacts_pull_openflow_instructions(struct ofpbuf *openflow,
         ogt->table_id = oigt->table_id;
     }
 
-    ofpact_pad(ofpacts);
-
     error = ofpacts_verify(ofpacts->data, ofpacts->size,
                            (1u << N_OVS_INSTRUCTIONS) - 1, 0);
 exit:
@@ -7272,7 +7262,6 @@ ofpact_put(struct ofpbuf *ofpacts, enum ofpact_type type, size_t len)
 {
     struct ofpact *ofpact;
 
-    ofpact_pad(ofpacts);
     ofpacts->header = ofpbuf_put_uninit(ofpacts, len);
     ofpact = ofpacts->header;
     ofpact_init(ofpact, type, len);
@@ -7288,41 +7277,18 @@ ofpact_init(struct ofpact *ofpact, enum ofpact_type type, size_t len)
     ofpact->len = len;
 }
 \f
-/* Updates 'ofpact->len' to the number of bytes in the tail of 'ofpacts'
- * starting at 'ofpact'.
- *
- * This is the correct way to update a variable-length ofpact's length after
- * adding the variable-length part of the payload.  (See the large comment
- * near the end of ofp-actions.h for more information.) */
+/* Finishes composing a variable-length action (begun using
+ * ofpact_put_<NAME>()), by padding the action to a multiple of OFPACT_ALIGNTO
+ * bytes and updating its embedded length field.  See the large comment near
+ * the end of ofp-actions.h for more information. */
 void
 ofpact_update_len(struct ofpbuf *ofpacts, struct ofpact *ofpact)
 {
     ovs_assert(ofpact == ofpacts->header);
     ofpact->len = (char *) ofpbuf_tail(ofpacts) - (char *) ofpact;
-}
-
-/* Pads out 'ofpacts' to a multiple of OFPACT_ALIGNTO bytes in length.  Each
- * ofpact_put_<ENUM>() calls this function automatically beforehand, but the
- * client must call this itself after adding the final ofpact to an array of
- * them.
- *
- * (The consequences of failing to call this function are probably not dire.
- * OFPACT_FOR_EACH will calculate a pointer beyond the end of the ofpacts, but
- * not dereference it.  That's undefined behavior, technically, but it will not
- * cause a real problem on common systems.  Still, it seems better to call
- * it.) */
-void
-ofpact_pad(struct ofpbuf *ofpacts)
-{
-    unsigned int pad = PAD_SIZE(ofpacts->size, OFPACT_ALIGNTO);
-    if (pad) {
-        ofpbuf_put_zeros(ofpacts, pad);
-    }
+    ofpbuf_padto(ofpacts, OFPACT_ALIGN(ofpacts->size));
 }
 \f
-
-
-
 static char * OVS_WARN_UNUSED_RESULT
 ofpact_parse(enum ofpact_type type, char *value, struct ofpbuf *ofpacts,
              enum ofputil_protocol *usable_protocols)
@@ -7426,7 +7392,6 @@ ofpacts_parse__(char *str, struct ofpbuf *ofpacts,
         }
         prev_inst = inst;
     }
-    ofpact_pad(ofpacts);
 
     if (drop && ofpacts->size) {
         return xstrdup("\"drop\" must not be accompanied by any other action "
index 18c7395..0bd33e3 100644 (file)
@@ -896,15 +896,16 @@ void *ofpact_put(struct ofpbuf *, enum ofpact_type, size_t len);
  *
  *   struct <STRUCT> *ofpact_put_<ENUM>(struct ofpbuf *ofpacts);
  *
- *     Appends a new 'ofpact', of length OFPACT_<ENUM>_RAW_SIZE, to 'ofpacts',
+ *     Appends a new 'ofpact', of length OFPACT_<ENUM>_SIZE, to 'ofpacts',
  *     initializes it with ofpact_init_<ENUM>(), and returns it.  Also sets
  *     'ofpacts->header' to the returned action.
  *
  *     After using this function to add a variable-length action, add the
  *     elements of the flexible array (e.g. with ofpbuf_put()), then use
- *     ofpact_update_len() to update the length embedded into the action.
- *     (Keep in mind the need to refresh the structure from 'ofpacts->frame'
- *     after adding data to 'ofpacts'.)
+ *     ofpact_update_len() to pad the action to a multiple of OFPACT_ALIGNTO
+ *     bytes and update its embedded length field.  (Keep in mind the need to
+ *     refresh the structure from 'ofpacts->header' after adding data to
+ *     'ofpacts'.)
  *
  *   struct <STRUCT> *ofpact_get_<ENUM>(const struct ofpact *ofpact);
  *
@@ -916,29 +917,22 @@ void *ofpact_put(struct ofpbuf *, enum ofpact_type, size_t len);
  *   void ofpact_init_<ENUM>(struct <STRUCT> *ofpact);
  *
  *     Initializes the parts of 'ofpact' that identify it as having type
- *     OFPACT_<ENUM> and length OFPACT_<ENUM>_RAW_SIZE and zeros the rest.
- *
- *   <ENUM>_RAW_SIZE
- *
- *     The size of the action structure.  For a fixed-length action, this is
- *     sizeof(struct <STRUCT>).  For a variable-length action, this is the
- *     offset to the variable-length part.
+ *     OFPACT_<ENUM> and length OFPACT_<ENUM>_SIZE and zeros the rest.
  *
  *   <ENUM>_SIZE
  *
- *     An integer constant, the value of OFPACT_<ENUM>_RAW_SIZE rounded up to a
- *     multiple of OFPACT_ALIGNTO.
+ *     The size of the action structure.  For a fixed-length action, this is
+ *     sizeof(struct <STRUCT>) rounded up to a multiple of OFPACT_ALIGNTO.  For
+ *     a variable-length action, this is the offset to the variable-length
+ *     part.
  */
 #define OFPACT(ENUM, STRUCT, MEMBER, NAME)                              \
     BUILD_ASSERT_DECL(offsetof(struct STRUCT, ofpact) == 0);            \
                                                                         \
-    enum { OFPACT_##ENUM##_RAW_SIZE                                     \
+    enum { OFPACT_##ENUM##_SIZE                                         \
            = (offsetof(struct STRUCT, MEMBER)                           \
               ? offsetof(struct STRUCT, MEMBER)                         \
-              : sizeof(struct STRUCT)) };                               \
-                                                                        \
-    enum { OFPACT_##ENUM##_SIZE                                         \
-           = ROUND_UP(OFPACT_##ENUM##_RAW_SIZE, OFPACT_ALIGNTO) };      \
+              : OFPACT_ALIGN(sizeof(struct STRUCT))) };                 \
                                                                         \
     static inline struct STRUCT *                                       \
     ofpact_get_##ENUM(const struct ofpact *ofpact)                      \
@@ -951,21 +945,20 @@ void *ofpact_put(struct ofpbuf *, enum ofpact_type, size_t len);
     ofpact_put_##ENUM(struct ofpbuf *ofpacts)                           \
     {                                                                   \
         return ofpact_put(ofpacts, OFPACT_##ENUM,                       \
-                          OFPACT_##ENUM##_RAW_SIZE);                    \
+                          OFPACT_##ENUM##_SIZE);                        \
     }                                                                   \
                                                                         \
     static inline void                                                  \
     ofpact_init_##ENUM(struct STRUCT *ofpact)                           \
     {                                                                   \
         ofpact_init(&ofpact->ofpact, OFPACT_##ENUM,                     \
-                    OFPACT_##ENUM##_RAW_SIZE);                          \
+                    OFPACT_##ENUM##_SIZE);                              \
     }
 OFPACTS
 #undef OFPACT
 
-/* Functions to use after adding ofpacts to a buffer. */
+/* Call after adding the variable-length part to a variable-length action. */
 void ofpact_update_len(struct ofpbuf *, struct ofpact *);
-void ofpact_pad(struct ofpbuf *);
 
 /* Additional functions for composing ofpacts. */
 struct ofpact_set_field *ofpact_put_reg_load(struct ofpbuf *ofpacts);
index ef2c06f..9a4da55 100644 (file)
@@ -2062,7 +2062,6 @@ connmgr_flushed(struct connmgr *mgr)
 
         ofpbuf_init(&ofpacts, OFPACT_OUTPUT_SIZE);
         ofpact_put_OUTPUT(&ofpacts)->port = OFPP_NORMAL;
-        ofpact_pad(&ofpacts);
 
         match_init_catchall(&match);
         ofproto_add_flow(mgr->ofproto, &match, 0, ofpacts.data,
index 38e775a..3c9a9ad 100644 (file)
@@ -229,7 +229,6 @@ fail_open_flushed(struct fail_open *fo)
          * OFPP_NORMAL. */
         ofpbuf_init(&ofpacts, OFPACT_OUTPUT_SIZE);
         ofpact_put_OUTPUT(&ofpacts)->port = OFPP_NORMAL;
-        ofpact_pad(&ofpacts);
 
         match_init_catchall(&match);
         ofproto_add_flow(fo->ofproto, &match, FAIL_OPEN_PRIORITY,
index dab64b9..57d877f 100644 (file)
@@ -4100,7 +4100,6 @@ xlate_write_actions(struct xlate_ctx *ctx, const struct ofpact *a)
     }
 
     ofpbuf_put(&ctx->action_set, on->actions, on_len);
-    ofpact_pad(&ctx->action_set);
 }
 
 static void
index 0a9ba4d..44e7bbc 100644 (file)
@@ -1407,7 +1407,6 @@ add_internal_flows(struct ofproto_dpif *ofproto)
     controller->max_len = UINT16_MAX;
     controller->controller_id = 0;
     controller->reason = OFPR_NO_MATCH;
-    ofpact_pad(&ofpacts);
 
     error = add_internal_miss_flow(ofproto, id++, &ofpacts,
                                    &ofproto->miss_rule);