> On Jun 17, 2016, at 10:16 PM, Russell Bryant <[email protected]> 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 <[email protected]>
Thanks,
--Justin
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
