| Message ID | 1466194603-20955-2-git-send-email-russell@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
> On Jun 17, 2016, at 10:16 PM, Russell Bryant <russell@ovn.org> wrote: > > diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c > index 5f0bf19..495d502 100644 > --- a/ovn/lib/actions.c > +++ b/ovn/lib/actions.c > @@ -414,7 +414,9 @@ parse_put_arp_action(struct action_context *ctx) > } > > static void > -emit_ct(struct action_context *ctx, bool recirc_next, bool commit) > +emit_ct(struct action_context *ctx, bool recirc_next, bool commit, > + int *ct_mark, int *ct_mark_mask, > + ovs_be128 *ct_label, ovs_be128 *ct_label_mask) It doesn't really matter, but the functions introduced here use signed numbers for mark, but unsigned for labels, so it's a bit inconsistent. Obviously sparse will hopefully catch any problems. > +/* Parse an argument to the ct_commit(); action. Supported arguments include: > + * > + * ct_mark=0 > + * ct_label=0 I wonder if it would make it clearer that this supports a mask. For example "ct_mark=<value>[/<mask>]". > + * > + * If a comma separates the current argument from the next argument, this > + * function will consume it. It may be worth mentioning that "mark_mask" and "label_mask" label need to be initialized. Another option would be to set the mask to an appropriate value in the case where a mask isn't supplied in the string. > + * > + * Return true after successfully parsing an argument. false on failure. */ > +static bool > +parse_ct_commit_arg(struct action_context *ctx, > + bool *set_mark, int *mark_value, int *mark_mask, > + bool *set_label, ovs_be128 *label_value, > + ovs_be128 *label_mask) > +{ > + if (lexer_match_id(ctx->lexer, "ct_mark")) { > + if (!lexer_match(ctx->lexer, LEX_T_EQUALS)) { > + action_error(ctx, "Expected '=' after argument to ct_commit"); > + return false; > + } > + if (ctx->lexer->token.type == LEX_T_INTEGER) { > + *mark_value = ntohll(ctx->lexer->token.value.integer); > + } else if (ctx->lexer->token.type == LEX_T_MASKED_INTEGER) { > + *mark_value = ntohll(ctx->lexer->token.value.integer); > + *mark_mask = ntohll(ctx->lexer->token.mask.integer); > + } else { > + action_error(ctx, "Expected integer after 'ct_mark=', got type %d", ctx->lexer->token.type); > + return false; > + } > + lexer_get(ctx->lexer); > + *set_mark = true; > + } else if (lexer_match_id(ctx->lexer, "ct_label")) { > + if (!lexer_match(ctx->lexer, LEX_T_EQUALS)) { > + action_error(ctx, "Expected '=' after argument to ct_commit"); > + return false; > + } > + if (ctx->lexer->token.type == LEX_T_INTEGER) { > + label_value->be64.lo = ctx->lexer->token.value.integer; > + } else if (ctx->lexer->token.type == LEX_T_MASKED_INTEGER) { > + label_value->be64.lo = ctx->lexer->token.value.integer; > + label_mask->be64.hi = 0; > + label_mask->be64.lo = ctx->lexer->token.mask.integer; > + } else { > + action_error(ctx, "Expected integer after 'ct_label='"); In the ct_mark error, it prints that type that was returned, but this doesn't. Is there a reason it's different? > --- a/ovn/ovn-sb.xml > +++ b/ovn/ovn-sb.xml > @@ -939,15 +939,30 @@ > </dd> > > <dt><code>ct_commit;</code></dt> > + <dt><code>ct_commit(ct_mark=<var>value</var>);</code></dt> > + <dt><code>ct_commit(ct_label=<var>value</var>);</code></dt> > + <dt><code>ct_commit(ct_mark=<var>value</var>, ct_label=<var>value</var>);</code></dt> I think it might be nice to be more explicit that masks are supported. For example, the documentation in the ovs-ofctl man page for ct_mark and ct_label show this with "=value[/mask]". > <dd> > <p> > - Commit the flow to the connection tracking entry associated > - with it by a previous call to <code>ct_next</code>. > + Commit the flow to the connection tracking entry associated with it > + by a previous call to <code>ct_next</code>. When > + <code>ct_mark=<var>value</var></code> and/or > + <code>ct_labe=<var>value</var></code> are supplied, s/ct_labe/ct_label/ > + <code>ct_mark</code> and/or <code>ct_label</code> will be set to the > + 32-bit integer indicated by <var>value</var> on the connection > + tracking entry. <var>value</var> may either be specified as an integer This mentions 32-bit integer, but can't labels be 128 bits? Acked-by: Justin Pettit <jpettit@ovn.org> Thanks, --Justin
On Mon, Jun 27, 2016 at 6:43 AM, Justin Pettit <jpettit@ovn.org> wrote: > > > On Jun 17, 2016, at 10:16 PM, Russell Bryant <russell@ovn.org> wrote: > > > > diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c > > index 5f0bf19..495d502 100644 > > --- a/ovn/lib/actions.c > > +++ b/ovn/lib/actions.c > > @@ -414,7 +414,9 @@ parse_put_arp_action(struct action_context *ctx) > > } > > > > static void > > -emit_ct(struct action_context *ctx, bool recirc_next, bool commit) > > +emit_ct(struct action_context *ctx, bool recirc_next, bool commit, > > + int *ct_mark, int *ct_mark_mask, > > + ovs_be128 *ct_label, ovs_be128 *ct_label_mask) > > It doesn't really matter, but the functions introduced here use signed > numbers for mark, but unsigned for labels, so it's a bit inconsistent. > Obviously sparse will hopefully catch any problems. > Thanks. I just did a build with sparse and didn't get any errors, at least. > > > +/* Parse an argument to the ct_commit(); action. Supported arguments > include: > > + * > > + * ct_mark=0 > > + * ct_label=0 > > I wonder if it would make it clearer that this supports a mask. For > example "ct_mark=<value>[/<mask>]". Good suggestion. I adopted it. The comment was from before I added mask support and I missed updating it. > > > + * > > + * If a comma separates the current argument from the next argument, > this > > + * function will consume it. > > It may be worth mentioning that "mark_mask" and "label_mask" label need > to be initialized. Another option would be to set the mask to an > appropriate value in the case where a mask isn't supplied in the string. > Good point. I will update the comment to clarify that the caller is expected to initialize the mask arguments. > > > + * > > + * Return true after successfully parsing an argument. false on > failure. */ > > +static bool > > +parse_ct_commit_arg(struct action_context *ctx, > > + bool *set_mark, int *mark_value, int *mark_mask, > > + bool *set_label, ovs_be128 *label_value, > > + ovs_be128 *label_mask) > > +{ > > + if (lexer_match_id(ctx->lexer, "ct_mark")) { > > + if (!lexer_match(ctx->lexer, LEX_T_EQUALS)) { > > + action_error(ctx, "Expected '=' after argument to > ct_commit"); > > + return false; > > + } > > + if (ctx->lexer->token.type == LEX_T_INTEGER) { > > + *mark_value = ntohll(ctx->lexer->token.value.integer); > > + } else if (ctx->lexer->token.type == LEX_T_MASKED_INTEGER) { > > + *mark_value = ntohll(ctx->lexer->token.value.integer); > > + *mark_mask = ntohll(ctx->lexer->token.mask.integer); > > + } else { > > + action_error(ctx, "Expected integer after 'ct_mark=', got > type %d", ctx->lexer->token.type); > > + return false; > > + } > > + lexer_get(ctx->lexer); > > + *set_mark = true; > > + } else if (lexer_match_id(ctx->lexer, "ct_label")) { > > + if (!lexer_match(ctx->lexer, LEX_T_EQUALS)) { > > + action_error(ctx, "Expected '=' after argument to > ct_commit"); > > + return false; > > + } > > + if (ctx->lexer->token.type == LEX_T_INTEGER) { > > + label_value->be64.lo = ctx->lexer->token.value.integer; > > + } else if (ctx->lexer->token.type == LEX_T_MASKED_INTEGER) { > > + label_value->be64.lo = ctx->lexer->token.value.integer; > > + label_mask->be64.hi = 0; > > + label_mask->be64.lo = ctx->lexer->token.mask.integer; > > + } else { > > + action_error(ctx, "Expected integer after 'ct_label='"); > > In the ct_mark error, it prints that type that was returned, but this > doesn't. Is there a reason it's different? > Not a good reason. :-) I had added the type value to the other message while debugging at one point. I think I will just remove it from the other message. The integer won't mean anything to an admin reading the log. > > > --- a/ovn/ovn-sb.xml > > +++ b/ovn/ovn-sb.xml > > @@ -939,15 +939,30 @@ > > </dd> > > > > <dt><code>ct_commit;</code></dt> > > + <dt><code>ct_commit(ct_mark=<var>value</var>);</code></dt> > > + <dt><code>ct_commit(ct_label=<var>value</var>);</code></dt> > > + <dt><code>ct_commit(ct_mark=<var>value</var>, > ct_label=<var>value</var>);</code></dt> > > I think it might be nice to be more explicit that masks are supported. > For example, the documentation in the ovs-ofctl man page for ct_mark and > ct_label show this with "=value[/mask]". > Agreed. This is another spot I missed updating when adding mask support. > > > <dd> > > <p> > > - Commit the flow to the connection tracking entry associated > > - with it by a previous call to <code>ct_next</code>. > > + Commit the flow to the connection tracking entry associated > with it > > + by a previous call to <code>ct_next</code>. When > > + <code>ct_mark=<var>value</var></code> and/or > > + <code>ct_labe=<var>value</var></code> are supplied, > > s/ct_labe/ct_label/ > Fixed. > > > + <code>ct_mark</code> and/or <code>ct_label</code> will be > set to the > > + 32-bit integer indicated by <var>value</var> on the > connection > > + tracking entry. <var>value</var> may either be specified as > an integer > > This mentions 32-bit integer, but can't labels be 128 bits? > Oops, yeah ... I think this is left over from when it was ct_mark only which is 32-bits. The code actually currently only handles 64-bits for ct_label, because that's what the OVN lexer supports for integer fields. I had a todo at one point to make use of parse_int_string() to support the full 128-bits. I've now clarified this doc, added an inline code comment about it, and added it to ovn/TODO to make sure I come back and fix the 128-bit todo item. > Acked-by: Justin Pettit <jpettit@ovn.org> > Thank you for the review. I really appreciate the attention to detail. I applied this patch to master.
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c index 5f0bf19..495d502 100644 --- a/ovn/lib/actions.c +++ b/ovn/lib/actions.c @@ -414,7 +414,9 @@ parse_put_arp_action(struct action_context *ctx) } static void -emit_ct(struct action_context *ctx, bool recirc_next, bool commit) +emit_ct(struct action_context *ctx, bool recirc_next, bool commit, + int *ct_mark, int *ct_mark_mask, + ovs_be128 *ct_label, ovs_be128 *ct_label_mask) { struct ofpact_conntrack *ct = ofpact_put_CT(ctx->ofpacts); ct->flags |= commit ? NX_CT_F_COMMIT : 0; @@ -440,6 +442,128 @@ emit_ct(struct action_context *ctx, bool recirc_next, bool commit) /* CT only works with IP, so set up a prerequisite. */ add_prerequisite(ctx, "ip"); + + size_t set_field_offset = ctx->ofpacts->size; + ofpbuf_pull(ctx->ofpacts, set_field_offset); + + if (ct_mark) { + struct ofpact_set_field *sf = ofpact_put_SET_FIELD(ctx->ofpacts); + sf->field = mf_from_id(MFF_CT_MARK); + sf->value.be32 = htonl(*ct_mark); + sf->mask.be32 = ct_mark_mask ? htonl(*ct_mark_mask) : OVS_BE32_MAX; + } + + if (ct_label) { + struct ofpact_set_field *sf = ofpact_put_SET_FIELD(ctx->ofpacts); + sf->field = mf_from_id(MFF_CT_LABEL); + sf->value.be128 = *ct_label; + sf->mask.be128 = ct_label_mask ? *ct_label_mask : OVS_BE128_MAX; + } + + ctx->ofpacts->header = ofpbuf_push_uninit(ctx->ofpacts, set_field_offset); + ct = ctx->ofpacts->header; + ofpact_finish(ctx->ofpacts, &ct->ofpact); +} + +/* Parse an argument to the ct_commit(); action. Supported arguments include: + * + * ct_mark=0 + * ct_label=0 + * + * If a comma separates the current argument from the next argument, this + * function will consume it. + * + * Return true after successfully parsing an argument. false on failure. */ +static bool +parse_ct_commit_arg(struct action_context *ctx, + bool *set_mark, int *mark_value, int *mark_mask, + bool *set_label, ovs_be128 *label_value, + ovs_be128 *label_mask) +{ + if (lexer_match_id(ctx->lexer, "ct_mark")) { + if (!lexer_match(ctx->lexer, LEX_T_EQUALS)) { + action_error(ctx, "Expected '=' after argument to ct_commit"); + return false; + } + if (ctx->lexer->token.type == LEX_T_INTEGER) { + *mark_value = ntohll(ctx->lexer->token.value.integer); + } else if (ctx->lexer->token.type == LEX_T_MASKED_INTEGER) { + *mark_value = ntohll(ctx->lexer->token.value.integer); + *mark_mask = ntohll(ctx->lexer->token.mask.integer); + } else { + action_error(ctx, "Expected integer after 'ct_mark=', got type %d", ctx->lexer->token.type); + return false; + } + lexer_get(ctx->lexer); + *set_mark = true; + } else if (lexer_match_id(ctx->lexer, "ct_label")) { + if (!lexer_match(ctx->lexer, LEX_T_EQUALS)) { + action_error(ctx, "Expected '=' after argument to ct_commit"); + return false; + } + if (ctx->lexer->token.type == LEX_T_INTEGER) { + label_value->be64.lo = ctx->lexer->token.value.integer; + } else if (ctx->lexer->token.type == LEX_T_MASKED_INTEGER) { + label_value->be64.lo = ctx->lexer->token.value.integer; + label_mask->be64.hi = 0; + label_mask->be64.lo = ctx->lexer->token.mask.integer; + } else { + action_error(ctx, "Expected integer after 'ct_label='"); + return false; + } + lexer_get(ctx->lexer); + *set_label = true; + } else { + action_error(ctx, "Expected argument to ct_commit()"); + return false; + } + + if (lexer_match(ctx->lexer, LEX_T_COMMA)) { + /* A comma is valid after an argument, but only if another + * argument is present (not a closing paren) */ + if (lexer_lookahead(ctx->lexer) == LEX_T_RPAREN) { + action_error(ctx, "Another argument to ct_commit() expected " + "after comma."); + return false; + } + } + + return true; +} + +static void +parse_ct_commit_action(struct action_context *ctx) +{ + if (!lexer_match(ctx->lexer, LEX_T_LPAREN)) { + /* ct_commit; */ + emit_ct(ctx, false, true, NULL, NULL, NULL, NULL); + return; + } + + /* ct_commit(); + * ct_commit(ct_mark=0); + * ct_commit(ct_label=0); + * ct_commit(ct_mark=0, ct_label=0); */ + + bool set_mark = false; + bool set_label = false; + int mark_value = 0; + int mark_mask = ~0; + ovs_be128 label_value = { .be32 = { 0, }, }; + ovs_be128 label_mask = OVS_BE128_MAX; + + while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) { + if (!parse_ct_commit_arg(ctx, &set_mark, &mark_value, &mark_mask, + &set_label, &label_value, &label_mask)) { + return; + } + } + + emit_ct(ctx, false, true, + set_mark ? &mark_value : NULL, + set_mark ? &mark_mask : NULL, + set_label ? &label_value : NULL, + set_label ? &label_mask : NULL); } static bool @@ -466,9 +590,9 @@ parse_action(struct action_context *ctx) action_syntax_error(ctx, "expecting `--'"); } } else if (lexer_match_id(ctx->lexer, "ct_next")) { - emit_ct(ctx, true, false); + emit_ct(ctx, true, false, NULL, NULL, NULL, NULL); } else if (lexer_match_id(ctx->lexer, "ct_commit")) { - emit_ct(ctx, false, true); + parse_ct_commit_action(ctx); } else if (lexer_match_id(ctx->lexer, "arp")) { parse_arp_action(ctx); } else if (lexer_match_id(ctx->lexer, "get_arp")) { diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml index d877f76..8eb7da7 100644 --- a/ovn/ovn-sb.xml +++ b/ovn/ovn-sb.xml @@ -939,15 +939,30 @@ </dd> <dt><code>ct_commit;</code></dt> + <dt><code>ct_commit(ct_mark=<var>value</var>);</code></dt> + <dt><code>ct_commit(ct_label=<var>value</var>);</code></dt> + <dt><code>ct_commit(ct_mark=<var>value</var>, ct_label=<var>value</var>);</code></dt> <dd> <p> - Commit the flow to the connection tracking entry associated - with it by a previous call to <code>ct_next</code>. + Commit the flow to the connection tracking entry associated with it + by a previous call to <code>ct_next</code>. When + <code>ct_mark=<var>value</var></code> and/or + <code>ct_labe=<var>value</var></code> are supplied, + <code>ct_mark</code> and/or <code>ct_label</code> will be set to the + 32-bit integer indicated by <var>value</var> on the connection + tracking entry. <var>value</var> may either be specified as an integer + (<code>1</code>) or an integer followed by a mask, indicating that only + a subset of bits will be set (<code>1/1</code>). </p> + <p> Note that if you want processing to continue in the next table, you must execute the <code>next</code> action after - <code>ct_commit</code>. + <code>ct_commit</code>. You may also leave out <code>next</code> + which will commit connection tracking state, and then drop the + packet. This could be useful for setting <code>ct_mark</code> + on a connection tracking entry before dropping a packet, + for example. </p> </dd> diff --git a/tests/ovn.at b/tests/ovn.at index a24e774..8aa78b9 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -506,6 +506,12 @@ ip.ttl => Syntax error at end of input expecting `--'. # conntrack ct_next; => actions=ct(table=27,zone=NXM_NX_REG5[0..15]), prereqs=ip ct_commit; => actions=ct(commit,zone=NXM_NX_REG5[0..15]), prereqs=ip +ct_commit(); => actions=ct(commit,zone=NXM_NX_REG5[0..15]), prereqs=ip +ct_commit(ct_mark=1); => actions=ct(commit,zone=NXM_NX_REG5[0..15],exec(set_field:0x1->ct_mark)), prereqs=ip +ct_commit(ct_mark=1/1); => actions=ct(commit,zone=NXM_NX_REG5[0..15],exec(set_field:0x1/0x1->ct_mark)), prereqs=ip +ct_commit(ct_label=1); => actions=ct(commit,zone=NXM_NX_REG5[0..15],exec(set_field:0x1->ct_label)), prereqs=ip +ct_commit(ct_label=1/1); => actions=ct(commit,zone=NXM_NX_REG5[0..15],exec(set_field:0x1/0x1->ct_label)), prereqs=ip +ct_commit(ct_mark=1, ct_label=2); => actions=ct(commit,zone=NXM_NX_REG5[0..15],exec(set_field:0x1->ct_mark,set_field:0x2->ct_label)), prereqs=ip # arp arp { eth.dst = ff:ff:ff:ff:ff:ff; output; }; => actions=controller(userdata=00.00.00.00.00.00.00.00.00.19.00.10.80.00.06.06.ff.ff.ff.ff.ff.ff.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00), prereqs=ip4
Update the "ct_commit;" logical flow action to optionally take one or two parameters, setting the value of "ct_mark" or "ct_label". Supported ct_commit syntax now includes: ct_commit; ct_commit(); ct_commit(ct_mark=1); ct_commit(ct_mark=1/1); ct_commit(ct_label=1); ct_commit(ct_label=1/1); ct_commit(ct_mark=1, ct_label=1); Setting ct_mark via this type of logical flow results in an OpenFlow flow that looks like: actions=ct(commit,zone=NXM_NX_REG5[0..15],exec(set_field:0x1->ct_mark)) Similarly, setting ct_label results in: actions=ct(commit,zone=NXM_NX_REG5[0..15],exec(set_field:0x1->ct_label)) A future commit will make use of this feature. Signed-off-by: Russell Bryant <russell@ovn.org> Acked-by: Ben Pfaff <blp@ovn.org> --- ovn/lib/actions.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- ovn/ovn-sb.xml | 21 +++++++-- tests/ovn.at | 6 +++ 3 files changed, 151 insertions(+), 6 deletions(-)