On Mon, Mar 21, 2016 at 10:54:58AM -0400, 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);
>
> 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 <[email protected]>
I have one comment to add to Guru's.
I think that the complicated dance around adding actions to set ct_mark
and ct_label could be done just once, like this:
size_t set_field_offset = ctx->ofpacts->size;
ofpbuf_pull(ctx->ofpacts, set_field_offset);
if (ct_mark) {
struct ofpact_set_field *sf = ofpact_put_SET_FIELD(ctx->ofpacts);
sf->field = mf_from_id(MFF_CT_MARK);
sf->value.be32 = htonl(*ct_mark);
sf->mask.be32 = OVS_BE32_MAX;
}
if (ct_label) {
struct ofpact_set_field *sf = ofpact_put_SET_FIELD(ctx->ofpacts);
sf->field = mf_from_id(MFF_CT_LABEL);
sf->value.be128 = *ct_label;
sf->mask.be128 = OVS_BE128_MAX;
}
ctx->ofpacts->header = ofpbuf_push_uninit(ctx->ofpacts, set_field_offset);
ct = ctx->ofpacts->header;
ofpact_finish(ctx->ofpacts, &ct->ofpact);
I think that this is conceptually better because I'm a little
uncomfortable with the idea of ofpact_finish()ing an action more than
once. It's harmless in this case, but...
Although I'm acking this, please make sure that you work with Guru to
get it to a place you both are happy with; I'm not trying to "override"
him or anything.
Acked-by: Ben Pfaff <[email protected]>
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
