[ovs-dev] [PATCH v5 1/2] ovn: Add ct_commit(ct_mark=INT, ct_label=INT); action.
Russell Bryant
russell at ovn.org
Thu Jun 30 20:08:12 UTC 2016
On Mon, Jun 27, 2016 at 6:43 AM, Justin Pettit <jpettit at ovn.org> wrote:
>
> > On Jun 17, 2016, at 10:16 PM, Russell Bryant <russell at 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 at ovn.org>
>
Thank you for the review. I really appreciate the attention to detail.
I applied this patch to master.
--
Russell Bryant
More information about the dev
mailing list