On Fri, Apr 01, 2016 at 11:20:20AM -0700, Russell Bryant wrote:
> 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_label=1);
> ct_commit(ct_mark=1, ct_label=1);
I guess I must have reviewed this before but I have more comments now.
The documentation only describes two of the above variants.
The documentation formatting can be improved also, e.g.:
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index aa614fc..a1304c5 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -939,14 +939,14 @@
</dd>
<dt><code>ct_commit;</code></dt>
- <dt><code>ct_commit(ct_mark=VALUE);</code></dt>
+ <dt><code>ct_commit(ct_mark=<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>. When
- the ct_mark=VALUE parameter is supplied, ct_mark will be set
- to the 32-bit integer indicated by VALUE on the connection
- tracking entry.
+ 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> is supplied,
+ <cod>ct_mark</cod> will be set to the 32-bit integer indicated by
+ <var>value</var> on the connection tracking entry.
</p>
<p>
> We would like to eventually also allow setting ct_mark and ct_label with
> a masked value. There are currently problems with this that Joe is
> working on, so that support will be added later.
>
> 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 <russ...@ovn.org>
> Acked-by: Ben Pfaff <b...@ovn.org>
You could avoid the XXX here by just calling parse_int_string(), which
handles decimal and hex (even very long hex):
> + /* XXX We only support a ct_label value specified as decimal.
> + * ct_label is 128-bit, so we should eventually also support
> specifying
> + * full 128-bit values as hex. Hex support isn't really needed until
> + * we need more than 32 bits. */
> + int val;
> + if (!action_get_int(ctx, &val)) {
> + return false;
> + }
> + label_value->be32[3] = htonl(val);
You might find action_force_match() useful, although the messages you
used are more specific.
I think that you could drop the second "if" statement in
parse_ct_commit_action(), the one that checks for LEX_T_RPAREN, without
any behavioral change.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
