From 6813ee7c06fbd2496fb73edd54a462a9053c6161 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Thu, 3 Jan 2013 09:02:52 -0800 Subject: [PATCH] ofp-actions: Fix the check for instruction ordering and duplication. Open vSwitch enforces that, when instructions appear as Nicira extensions in OpenFlow 1.0 action lists, the instructions appear in the order that an OpenFlow 1.1+ switch would execute them and that no duplicates appear. But the check wasn't general enough, because it had been implemented when only one instruction was implemented and never later generalized. This commit fixes the problem. One of the tests was actually testing for the wrong behavior that the check implemented, so this commit corrects that test. It also updates another test with updated log messages. Reported-by: Jing Ai Tested-by: Jing Ai Signed-off-by: Ben Pfaff --- AUTHORS | 1 + lib/ofp-actions.c | 39 +++++++++++++++++++++++++++------------ tests/ofp-actions.at | 37 ++++++++++++++++++++++++++++++++----- 3 files changed, 60 insertions(+), 17 deletions(-) diff --git a/AUTHORS b/AUTHORS index 5d34fbe45..060fc19dc 100644 --- a/AUTHORS +++ b/AUTHORS @@ -133,6 +133,7 @@ Janis Hamme janis.hamme@student.kit.edu Jari Sundell sundell.software@gmail.com Jed Daniels openvswitch@jeddaniels.com Jeongkeun Lee jklee@hp.com +Jing Ai ai_jing2000@hotmail.com Joan Cirer joan@ev0.net John Galgay john@galgay.net Kevin Mancuso kevin.mancuso@rackspace.com diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index 6468cacf8..1bc8a9cd3 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc. + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -1153,23 +1153,38 @@ enum ofperr ofpacts_verify(const struct ofpact ofpacts[], size_t ofpacts_len) { const struct ofpact *a; - const struct ofpact_metadata *om = NULL; + enum ovs_instruction_type inst; + inst = OVSINST_OFPIT11_APPLY_ACTIONS; OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) { - if (om) { - if (a->type == OFPACT_WRITE_METADATA) { - VLOG_WARN("duplicate write_metadata instruction specified"); - return OFPERR_OFPBAC_UNSUPPORTED_ORDER; + enum ovs_instruction_type next; + + if (a->type == OFPACT_CLEAR_ACTIONS) { + next = OVSINST_OFPIT11_CLEAR_ACTIONS; + } else if (a->type == OFPACT_WRITE_METADATA) { + next = OVSINST_OFPIT11_WRITE_METADATA; + } else if (a->type == OFPACT_GOTO_TABLE) { + next = OVSINST_OFPIT11_GOTO_TABLE; + } else { + next = OVSINST_OFPIT11_APPLY_ACTIONS; + } + + if (inst != OVSINST_OFPIT11_APPLY_ACTIONS && next <= inst) { + const char *name = ofpact_instruction_name_from_type(inst); + const char *next_name = ofpact_instruction_name_from_type(next); + + if (next == inst) { + VLOG_WARN("duplicate %s instruction not allowed, for OpenFlow " + "1.1+ compatibility", name); } else { - VLOG_WARN("write_metadata instruction must be specified after " - "other instructions/actions"); - return OFPERR_OFPBAC_UNSUPPORTED_ORDER; + VLOG_WARN("invalid instruction ordering: %s must appear " + "before %s, for OpenFlow 1.1+ compatibility", + next_name, name); } + return OFPERR_OFPBAC_UNSUPPORTED_ORDER; } - if (a->type == OFPACT_WRITE_METADATA) { - om = (const struct ofpact_metadata *) a; - } + inst = next; } return 0; diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at index 30fcf51ff..aa51e0846 100644 --- a/tests/ofp-actions.at +++ b/tests/ofp-actions.at @@ -240,12 +240,12 @@ dnl action instead, so parse-ofp11-actions will recognise and drop this action. ffff 0020 00002320 0016 000000000000 fedcba9876543210 ffffffffffffffff dnl Write-Metadata duplicated. -& ofp_actions|WARN|duplicate write_metadata instruction specified +& ofp_actions|WARN|duplicate write_metadata instruction not allowed, for OpenFlow 1.1+ compatibility # bad OF1.1 actions: OFPBAC_UNSUPPORTED_ORDER ffff 0020 00002320 0016 000000000000 fedcba9876543210 ffffffffffffffff ffff 0020 00002320 0016 000000000000 fedcba9876543210 ffffffffffffffff dnl Write-Metadata in wrong position. -& ofp_actions|WARN|write_metadata instruction must be specified after other instructions/actions +& ofp_actions|WARN|invalid instruction ordering: apply_actions must appear before write_metadata, for OpenFlow 1.1+ compatibility # bad OF1.1 actions: OFPBAC_UNSUPPORTED_ORDER ffff 0020 00002320 0016 000000000000 fedcba9876543210 ffffffffffffffff ffff 0010 00002320 0002 0000 12345678 @@ -382,9 +382,36 @@ dnl Write-Metadata duplicated. # bad OF1.1 instructions: OFPBAC_UNSUPPORTED_ORDER 0002 0018 00000000 fedcba9876543210 ff00ff00ff00ff00 0002 0018 00000000 fedcba9876543210 ff00ff00ff00ff00 -dnl Write-Metadata in wrong position. -& ofp_actions|WARN|write_metadata instruction must be specified after other instructions/actions -# bad OF1.1 instructions: OFPBAC_UNSUPPORTED_ORDER +dnl Write-Metadata in wrong position (OpenFlow 1.1+ disregards the order +dnl and OVS reorders it to the canonical order) +# actions=write_metadata:0xfedcba9876543210,goto_table:1 +# 1: 01 -> 02 +# 3: 08 -> 18 +# 4: 01 -> 00 +# 8: 00 -> fe +# 9: 02 -> dc +# 10: 00 -> ba +# 11: 18 -> 98 +# 12: 00 -> 76 +# 13: 00 -> 54 +# 14: 00 -> 32 +# 15: 00 -> 10 +# 16: fe -> ff +# 17: dc -> ff +# 18: ba -> ff +# 19: 98 -> ff +# 20: 76 -> ff +# 21: 54 -> ff +# 22: 32 -> ff +# 23: 10 -> ff +# 24: ff -> 00 +# 25: ff -> 01 +# 26: ff -> 00 +# 27: ff -> 08 +# 28: ff -> 01 +# 29: ff -> 00 +# 30: ff -> 00 +# 31: ff -> 00 0001 0008 01 000000 0002 0018 00000000 fedcba9876543210 ffffffffffffffff dnl Write-Actions not supported yet. -- 2.47.0