No problem. I just pinged the thread to get attention. Thanks for your
review. Here I attached the patch regenerated by "git format-patch".
Signed-off-by: Jing Ai <ji...@google.com>
---
AUTHORS | 3 ++-
lib/ofp-actions.c | 5 +++++
lib/ofp-actions.h | 1 +
lib/ofp-util.c | 8 +++++---
tests/ofp-actions.at | 4 ++++
utilities/ovs-ofctl.c | 17 ++++++++++++++---
6 files changed, 31 insertions(+), 7 deletions(-)
diff --git a/AUTHORS b/AUTHORS
index d15173e..a2b0c4d 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -45,6 +45,7 @@ Jarno Rajahalme jarno.rajaha...@nsn.com
Jean Tourrilhes j...@hpl.hp.com
Jeremy Stribling st...@nicira.com
Jesse Gross je...@nicira.com
+Jing Ai ji...@google.com
Joe Perches j...@perches.com
Joe Stringer j...@wand.net.nz
Jun Nakajima jun.nakaj...@intel.com
@@ -153,7 +154,7 @@ Jari Sundell sundell.softw...@gmail.com
Jed Daniels openvswi...@jeddaniels.com
Jeff Merrick jmerr...@vmware.com
Jeongkeun Lee jk...@hp.com
-Jing Ai ai_jing2...@hotmail.com
+Jing Ai ji...@google.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 026a376..30f3f65 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1052,6 +1052,7 @@ ofpacts_pull_openflow11_actions(struct ofpbuf
*openflow,
enum ofperr
ofpacts_pull_openflow11_instructions(struct ofpbuf *openflow,
unsigned int instructions_len,
+ uint8_t table_id,
struct ofpbuf *ofpacts)
{
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
@@ -1119,6 +1120,10 @@ ofpacts_pull_openflow11_instructions(struct ofpbuf
*openflow,
oigt = instruction_get_OFPIT11_GOTO_TABLE(
insts[OVSINST_OFPIT11_GOTO_TABLE]);
+ if (table_id >= oigt->table_id) {
+ error = OFPERR_OFPBRC_BAD_TABLE_ID;
+ goto exit;
+ }
ogt = ofpact_put_GOTO_TABLE(ofpacts);
ogt->table_id = oigt->table_id;
}
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index ffceb05..9a74bcc 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -490,6 +490,7 @@ enum ofperr ofpacts_pull_openflow11_actions(struct
ofpbuf *openflow,
struct ofpbuf *ofpacts);
enum ofperr ofpacts_pull_openflow11_instructions(struct ofpbuf *openflow,
unsigned int
instructions_len,
+ uint8_t table_id,
struct ofpbuf *ofpacts);
enum ofperr ofpacts_check(const struct ofpact[], size_t ofpacts_len,
const struct flow *, int max_ports);
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 3c8d5a2..42021a8 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -1502,7 +1502,8 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
return error;
}
- error = ofpacts_pull_openflow11_instructions(&b, b.size, ofpacts);
+ error = ofpacts_pull_openflow11_instructions(&b, b.size,
ofm->table_id,
+ ofpacts);
if (error) {
return error;
}
@@ -1652,7 +1653,7 @@ ofputil_encode_flow_mod(const struct ofputil_flow_mod
*fm,
case OFPUTIL_P_OF13_OXM: {
struct ofp11_flow_mod *ofm;
- msg = ofpraw_alloc(OFPRAW_OFPT11_FLOW_MOD,
+ msg = ofpraw_alloc(OFPRAW_OFPT11_FLOW_MOD,
ofputil_protocol_to_ofp_version(protocol),
NXM_TYPICAL_LEN + fm->ofpacts_len);
ofm = ofpbuf_put_zeros(msg, sizeof *ofm);
@@ -2014,7 +2015,8 @@ ofputil_decode_flow_stats_reply(struct
ofputil_flow_stats *fs,
}
if (ofpacts_pull_openflow11_instructions(msg, length - sizeof *ofs
-
- padded_match_len,
ofpacts)) {
+ padded_match_len,
+ ofs->table_id, ofpacts)) {
VLOG_WARN_RL(&bad_ofmsg_rl, "OFPST_FLOW reply bad
instructions");
return EINVAL;
}
diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
index 2ecbdb5..b455bb9 100644
--- a/tests/ofp-actions.at
+++ b/tests/ofp-actions.at
@@ -356,6 +356,10 @@ dnl Goto-Table 1 instruction non-zero padding
# 7: 01 -> 00
0001 0008 01 000001
+dnl Goto-Table 1 instruction go back to the previous table.
+# bad OF1.1 instructions: OFPBRC_BAD_TABLE_ID
+2,0001 0008 01 000000
+
dnl Goto-Table 1
# actions=goto_table:1
0001 0008 01 000000
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 24b9434..009768a 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -2607,18 +2607,29 @@ ofctl_parse_ofp11_instructions(int argc OVS_UNUSED,
char *argv[] OVS_UNUSED)
enum ofperr error;
size_t size;
struct ds s;
+ const char *instructions;
+ const char *table_id;
+
+ /* Parse table_id separated with the follow-up instructions by
",", if
+ * any. */
+ instructions = ds_cstr(&in);
+ table_id = NULL;
+ if (strstr(instructions, ",")) {
+ table_id = strsep(&instructions, ",");
+ }
/* Parse hex bytes. */
ofpbuf_init(&of11_in, 0);
- if (ofpbuf_put_hex(&of11_in, ds_cstr(&in), NULL)[0] != '\0') {
+ if (ofpbuf_put_hex(&of11_in, instructions, NULL)[0] != '\0') {
ovs_fatal(0, "Trailing garbage in hex data");
}
/* Convert to ofpacts. */
ofpbuf_init(&ofpacts, 0);
size = of11_in.size;
- error = ofpacts_pull_openflow11_instructions(&of11_in,
of11_in.size,
- &ofpacts);
+ error = ofpacts_pull_openflow11_instructions(
+ &of11_in, of11_in.size, table_id ? (uint8_t)atoi(table_id) : 0,
+ &ofpacts);
if (error) {
printf("bad OF1.1 instructions: %s\n\n",
ofperr_get_name(error));
ofpbuf_uninit(&ofpacts);
--
1.8.3
On Tue, Jun 4, 2013 at 12:51 PM, Ben Pfaff <b...@nicira.com> wrote:
> I was out Friday through Monday, sorry.
>
> On Tue, Jun 04, 2013 at 10:33:53AM -0700, Jing Ai wrote:
> > Anyone reviewing it? Thanks!
> >
> > Best,
> > Jing
> >
> >
> > On Fri, May 31, 2013 at 5:46 PM, Jing Ai <ji...@google.com> wrote:
> >
> > > --- Resent the patch since the last one was messed up in format
> > > --- Added and changed AUTHOR information
> > >
> > > Found a bug that OVS does not return any error message when the
> > > goto_table_id is smaller than (or equals to) then current table where
> the
> > > flow is installed. As a result, I install a flow as follows (using
> > > "ovs-ofctl dump-flows br0")
> > >
> > > table_id=4, duration=40s, priority=32768, n_packets=0, n_bytes=0,
> > >
> tcp,in_port=1,dl_src=00:06:07:08:09:0a,dl_dst=00:01:02:03:04:05,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,nw_ecn=0,tp_src=1234,tp_dst=80,actions=goto_table:3
> > >
> > > Best,
> > > Jing
> > >
> > > Signed-off-by: Jing Ai <ji...@google.com>
> > > ---
> > > AUTHORS | 3 ++-
> > > lib/ofp-actions.c | 5 +++++
> > > lib/ofp-actions.h | 1 +
> > > lib/ofp-util.c | 8 +++++---
> > > tests/ofp-actions.at | 4 ++++
> > > utilities/ovs-ofctl.c | 17 ++++++++++++++---
> > > 6 files changed, 31 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/AUTHORS b/AUTHORS
> > > index d15173e..a2b0c4d 100644
> > > --- a/AUTHORS
> > > +++ b/AUTHORS
> > > @@ -45,6 +45,7 @@ Jarno Rajahalme jarno.rajaha...@nsn.com
> > > Jean Tourrilhes j...@hpl.hp.com
> > > Jeremy Stribling st...@nicira.com
> > > Jesse Gross je...@nicira.com
> > > +Jing Ai ji...@google.com
> > > Joe Perches j...@perches.com
> > > Joe Stringer j...@wand.net.nz
> > > Jun Nakajima jun.nakaj...@intel.com
> > > @@ -153,7 +154,7 @@ Jari Sundell sundell.softw...@gmail.com
> > > Jed Daniels openvswi...@jeddaniels.com
> > > Jeff Merrick jmerr...@vmware.com
> > > Jeongkeun Lee jk...@hp.com
> > > -Jing Ai ai_jing2...@hotmail.com
> > > +Jing Ai ji...@google.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 026a376..30f3f65 100644
> > > --- a/lib/ofp-actions.c
> > > +++ b/lib/ofp-actions.c
> > > @@ -1052,6 +1052,7 @@ ofpacts_pull_openflow11_actions(struct ofpbuf
> > > *openflow,
> > > enum ofperr
> > > ofpacts_pull_openflow11_instructions(struct ofpbuf *openflow,
> > > unsigned int instructions_len,
> > > + uint8_t table_id,
> > > struct ofpbuf *ofpacts)
> > > {
> > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > > @@ -1119,6 +1120,10 @@ ofpacts_pull_openflow11_instructions(struct
> ofpbuf
> > > *openflow,
> > >
> > > oigt = instruction_get_OFPIT11_GOTO_TABLE(
> > > insts[OVSINST_OFPIT11_GOTO_TABLE]);
> > > + if (table_id >= oigt->table_id) {
> > > + error = OFPERR_OFPBRC_BAD_TABLE_ID;
> > > + goto exit;
> > > + }
> > > ogt = ofpact_put_GOTO_TABLE(ofpacts);
> > > ogt->table_id = oigt->table_id;
> > > }
> > > diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
> > > index ffceb05..9a74bcc 100644
> > > --- a/lib/ofp-actions.h
> > > +++ b/lib/ofp-actions.h
> > > @@ -490,6 +490,7 @@ enum ofperr ofpacts_pull_openflow11_actions(struct
> > > ofpbuf *openflow,
> > > struct ofpbuf *ofpacts);
> > > enum ofperr ofpacts_pull_openflow11_instructions(struct ofpbuf
> *openflow,
> > > unsigned int
> > > instructions_len,
> > > + uint8_t table_id,
> > > struct ofpbuf
> *ofpacts);
> > > enum ofperr ofpacts_check(const struct ofpact[], size_t ofpacts_len,
> > > const struct flow *, int max_ports);
> > > diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> > > index 3c8d5a2..42021a8 100644
> > > --- a/lib/ofp-util.c
> > > +++ b/lib/ofp-util.c
> > > @@ -1502,7 +1502,8 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod
> *fm,
> > > return error;
> > > }
> > >
> > > - error = ofpacts_pull_openflow11_instructions(&b, b.size,
> ofpacts);
> > > + error = ofpacts_pull_openflow11_instructions(&b, b.size,
> > > ofm->table_id,
> > > + ofpacts);
> > > if (error) {
> > > return error;
> > > }
> > > @@ -1652,7 +1653,7 @@ ofputil_encode_flow_mod(const struct
> > > ofputil_flow_mod *fm,
> > > case OFPUTIL_P_OF13_OXM: {
> > > struct ofp11_flow_mod *ofm;
> > >
> > > - msg = ofpraw_alloc(OFPRAW_OFPT11_FLOW_MOD,
> > > + msg = ofpraw_alloc(OFPRAW_OFPT11_FLOW_MOD,
> > > ofputil_protocol_to_ofp_version(protocol),
> > > NXM_TYPICAL_LEN + fm->ofpacts_len);
> > > ofm = ofpbuf_put_zeros(msg, sizeof *ofm);
> > > @@ -2014,7 +2015,8 @@ ofputil_decode_flow_stats_reply(struct
> > > ofputil_flow_stats *fs,
> > > }
> > >
> > > if (ofpacts_pull_openflow11_instructions(msg, length - sizeof
> > > *ofs -
> > > - padded_match_len,
> > > ofpacts)) {
> > > + padded_match_len,
> > > + ofs->table_id,
> ofpacts))
> > > {
> > > VLOG_WARN_RL(&bad_ofmsg_rl, "OFPST_FLOW reply bad
> > > instructions");
> > > return EINVAL;
> > > }
> > > diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
> > > index 2ecbdb5..b455bb9 100644
> > > --- a/tests/ofp-actions.at
> > > +++ b/tests/ofp-actions.at
> > > @@ -356,6 +356,10 @@ dnl Goto-Table 1 instruction non-zero padding
> > > # 7: 01 -> 00
> > > 0001 0008 01 000001
> > >
> > > +dnl Goto-Table 1 instruction go back to the previous table.
> > > +# bad OF1.1 instructions: OFPBRC_BAD_TABLE_ID
> > > +2,0001 0008 01 000000
> > > +
> > > dnl Goto-Table 1
> > > # actions=goto_table:1
> > > 0001 0008 01 000000
> > > diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> > > index 24b9434..009768a 100644
> > > --- a/utilities/ovs-ofctl.c
> > > +++ b/utilities/ovs-ofctl.c
> > > @@ -2607,18 +2607,29 @@ ofctl_parse_ofp11_instructions(int argc
> > > OVS_UNUSED, char *argv[] OVS_UNUSED)
> > > enum ofperr error;
> > > size_t size;
> > > struct ds s;
> > > + const char *instructions;
> > > + const char *table_id;
> > > +
> > > + /* Parse table_id separated with the follow-up instructions by
> > > ",", if
> > > + * any. */
> > > + instructions = ds_cstr(&in);
> > > + table_id = NULL;
> > > + if (strstr(instructions, ",")) {
> > > + table_id = strsep(&instructions, ",");
> > > + }
> > >
> > > /* Parse hex bytes. */
> > > ofpbuf_init(&of11_in, 0);
> > > - if (ofpbuf_put_hex(&of11_in, ds_cstr(&in), NULL)[0] != '\0') {
> > > + if (ofpbuf_put_hex(&of11_in, instructions, NULL)[0] != '\0') {
> > > ovs_fatal(0, "Trailing garbage in hex data");
> > > }
> > >
> > > /* Convert to ofpacts. */
> > > ofpbuf_init(&ofpacts, 0);
> > > size = of11_in.size;
> > > - error = ofpacts_pull_openflow11_instructions(&of11_in,
> > > of11_in.size,
> > > - &ofpacts);
> > > + error = ofpacts_pull_openflow11_instructions(
> > > + &of11_in, of11_in.size, table_id ?
> (uint8_t)atoi(table_id) :
> > > 0,
> > > + &ofpacts);
> > > if (error) {
> > > printf("bad OF1.1 instructions: %s\n\n",
> > > ofperr_get_name(error));
> > > ofpbuf_uninit(&ofpacts);
> > > --
> > > 1.8.2.1
> > >
> > >
>
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
