Thanks Ben. You solution looks better.
I have no further comments on your codes and tests. Also, I have tested it
manually by running various combinations of instructions/actions and it works
as intended.
One more question, do you think we should normalize any action specified after
resubmit action? For example, actions="resubmit(,1)",write_metadata=<metadata>,
metadata will not be propagated. It is fine to do in a separate patch.
Best,Jing
> Date: Thu, 3 Jan 2013 09:03:50 -0800
> From: b...@nicira.com
> To: ai_jing2...@hotmail.com
> CC: dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] ofp-actions: Allow write_metadata instruction
> before goto_table instruction.
>
> On Thu, Jan 03, 2013 at 06:41:37AM +0000, Jing Ai wrote:
> > In the current codes, when running "ovs-ofctl add-flow br0 <match
> > conditions>,actions=write_metadata:<64-bit
> > metadata>,goto_table:<table_id>", it gives the following
> > error:2012-12-29T00:23:23Z|00001|ofp_actions|WARN|write_metadata
> > instruction must be specified after other
> > instructions/actionsovs-ofctl: Incorrect instruction orderingAccording
> > to OpenFlow spec 1.2, only goto_table instruction could go after
> > write_metadata instruction. This patch intends to address the above
> > issue.
>
> It doesn't generalize enough to all the possible order violations.
> Here's a more general version. Will you please verify that it also
> solves the problem you see?
>
> Thanks,
>
> Ben.
>
> --8<--------------------------cut here-------------------------->8--
>
> From: Ben Pfaff <b...@nicira.com>
> 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 <ai_jing2...@hotmail.com>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> AUTHORS | 1 +
> lib/ofp-actions.c | 38 ++++++++++++++++++++++++++------------
> tests/ofp-actions.at | 37 ++++++++++++++++++++++++++++++++-----
> 3 files changed, 59 insertions(+), 17 deletions(-)
>
> diff --git a/AUTHORS b/AUTHORS
> index 5d34fbe..060fc19 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -133,6 +133,7 @@ Janis Hamme janis.ha...@student.kit.edu
> Jari Sundell sundell.softw...@gmail.com
> Jed Daniels openvswi...@jeddaniels.com
> Jeongkeun Lee jk...@hp.com
> +Jing Ai ai_jing2...@hotmail.com
> Joan Cirer j...@ev0.net
> John Galgay j...@galgay.net
> Kevin Mancuso kevin.manc...@rackspace.com
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index 6468cac..37205b2 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,37 @@ 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 = 0;
>
> 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 30fcf51..aa51e08 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.
> --
> 1.7.10.4
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
