On Mon, Jun 27, 2016 at 6:43 AM, Justin Pettit <jpet...@ovn.org> wrote:
> > > On Jun 17, 2016, at 10:16 PM, Russell Bryant <russ...@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 <jpet...@ovn.org> > Thank you for the review. I really appreciate the attention to detail. I applied this patch to master. -- Russell Bryant _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev