On Oct 23, 2013, at 9:46 AM, Ben Pfaff <b...@nicira.com> wrote:

> On Tue, Oct 22, 2013 at 05:20:39PM -0700, Jarno Rajahalme wrote:
>> Make union ofp_action more generic by allowing it to be used for
>> all actions.  OFP_ACTION_ALIGN (which is 8, as the sizeof(union
>> ofp_action) used to be) is now used where sizeof union ofp_action
>> was used before.  On the same note, variable name 'n_actions' is
>> changed to 'max_actions' as the number of actions cannot be simply
>> divided when actions have various sizes.
>> 
>> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
> 
> I think that this pointer arithmetic in ofpacts_from_openflow() is now
> wrong:
>            log_bad_action(in, n_in, a - in, error);

Oops, would you be happy with this incremental:

diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 777677a..df88ce9 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -602,8 +602,8 @@ action_is_valid(const union ofp_action *a, size_t 
max_actions)
           (ITER) = action_next(ITER)))
 
 static void
-log_bad_action(const union ofp_action *actions, size_t max_actions, size_t ofs,
-               enum ofperr error)
+log_bad_action(const union ofp_action *actions, size_t max_actions,
+               const union ofp_action *bad_action, enum ofperr error)
 {
     if (!VLOG_DROP_WARN(&rl)) {
         struct ds s;
@@ -611,7 +611,8 @@ log_bad_action(const union ofp_action *actions, size_t 
max_actions, size_t ofs,
         ds_init(&s);
         ds_put_hex_dump(&s, actions, max_actions * OFP_ACTION_ALIGN, 0, false);
         VLOG_WARN("bad action at offset %#zx (%s):\n%s",
-                  ofs * OFP_ACTION_ALIGN, ofperr_get_name(error), ds_cstr(&s));
+                  (char *)bad_action - (char *)actions,
+                  ofperr_get_name(error), ds_cstr(&s));
         ds_destroy(&s);
     }
 }
@@ -628,13 +629,13 @@ ofpacts_from_openflow(const union ofp_action *in, size_t 
n_in,
     ACTION_FOR_EACH (a, left, in, n_in) {
         enum ofperr error = ofpact_from_openflow(a, out);
         if (error) {
-            log_bad_action(in, n_in, a - in, error);
+            log_bad_action(in, n_in, a, error);
             return error;
         }
     }
     if (left) {
         enum ofperr error = OFPERR_OFPBAC_BAD_LEN;
-        log_bad_action(in, n_in, n_in - left, error);
+        log_bad_action(in, n_in, a, error);
         return error;
     }
 

Jarno


_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to