[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