On Wed, Mar 23, 2016 at 4:48 PM, Ben Pfaff <b...@ovn.org> wrote:
> 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 <russ...@ovn.org>
>
> 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 <b...@ovn.org>
>
Thanks for the suggestion. I incorporated this for the next revision.
I also added tests.
--
Russell Bryant
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
