On Wed, Aug 15, 2012 at 12:28:47AM +1200, Joe Stringer wrote:
> In OpenFlow 1.1, we add support for OFPIT_WRITE_METADATA. This allows us to
> write to the metadata field. Internally it is represented using
> ofpact_metadata.
>
> We introduce NXAST_WRITE_METADATA to handle writing to the metadata field in
> OpenFlow 1.0+. This structure reflects OFPIT_WRITE_METADATA.
>
> When writing out the structure to OpenFlow 1.1, it uses the
> OFPIT_WRITE_METADATA
> instruction only, and not the new NXAST action (which would be redundant).
>
> Signed-off-by: Joe Stringer <[email protected]>
Isaku's comment seems reasonable to me.
> +/* Action structure for NXAST_WRITE_METADATA.
> + *
> + * Modifies the 'mask' bits of the metadata value. */
> +struct nx_action_write_metadata {
> + ovs_be16 type; /* OFPAT_VENDOR. */
> + ovs_be16 len; /* Length is 32. */
> + ovs_be32 vendor; /* NX_VENDOR_ID. */
> + ovs_be16 subtype; /* NXAST_WRITE_METADATA. */
> + uint8_t pad[6];
I've been trying to insist for newly added actions that any padding
bytes must be zero. So I'd rename this "zeros", add a comment "Must
be zero." and check for that in metadata_from_openflow(), returning
OFPERR_NXBRC_MUST_BE_ZERO if there is a nonzero byte.
> + ovs_be64 metadata; /* Metadata register. */
> + ovs_be64 mask; /* Metadata mask. */
> +};
> +OFP_ASSERT(sizeof(struct nx_action_write_metadata) == 32);
> + if (insts[OVSINST_OFPIT11_WRITE_METADATA]) {
> + const struct ofp11_instruction_write_metadata *oiwm;
> + struct ofpact_metadata *om;
> +
> + oiwm = (const struct ofp11_instruction_write_metadata *)
> + insts[OVSINST_OFPIT11_WRITE_METADATA];
> +
> + om = (struct ofpact_metadata *)ofpact_put_WRITE_METADATA(ofpacts);
Doesn't ofpact_put_WRITE_METADATA() return the correct type? (Why
not?)
> +/* Verifies that the 'ofpacts_len' bytes of actions in 'ofpacts' are
> + * in the appropriate order as defined by the OpenFlow spec. */
> +enum ofperr
> +ofpacts_verify(const struct ofpact ofpacts[], size_t ofpacts_len) {
> + const struct ofpact *a;
> + const struct ofpact_metadata *om = NULL;
> +
> + OFPACT_FOR_EACH(a, ofpacts, ofpacts_len) {
Please add a space before the (.
> + if (om) {
> + if (a->type == OFPACT_WRITE_METADATA) {
> + VLOG_WARN("Duplicate write_metadata instruction specified.");
> + return OFPERR_NXBIC_DUP_TYPE;
> + } else {
> + VLOG_WARN("write_metadata instruction must be specified
> after "
> + "other instructions/actions.");
> + return OFPERR_OFPBAC_UNSUPPORTED_ORDER;
It looks like the other log messages in this file typically do not
begin with a capital letter or end with a period.
I'd forgotten that there was an "unsupported order" error, but it is
certainly appropriate here. Good choice.
> + }
> + }
> +
> + if (a->type == OFPACT_WRITE_METADATA) {
> + om = (const struct ofpact_metadata *) a;
> + }
> + }
> +
> + return 0;
> +}
>
> /* Converting ofpacts to Nicira OpenFlow extensions. */
>
> @@ -1524,6 +1599,14 @@ ofpacts_put_openflow11_instructions(const struct
> ofpact ofpacts[],
> in_instruction = true;
> ofs = openflow->size;
> instruction_put_OFPIT11_WRITE_ACTIONS(openflow);
> + } else if (a->type == OFPACT_WRITE_METADATA) {
> + const struct ofpact_metadata *om;
> + struct ofp11_instruction_write_metadata *oiwm;
> + om = ofpact_get_WRITE_METADATA(a);
> + oiwm = instruction_put_OFPIT11_WRITE_METADATA(openflow);
> + oiwm->metadata = htonll(om->metadata);
> + oiwm->metadata_mask = htonll(om->mask);
> + memset(oiwm->pad, 0, sizeof oiwm->pad);
instruction_put_*() zeros what it adds, so I don't think the memset is
necessary here.
> +dnl OpenFlow 1.1 uses OFPIT_WRITE_METADATA to express the
> NXAST_WRITE_METADATA
> +dnl action instead, so parse-ofp11-actions will recognise and drop this
> action.
> +# instructions=write_metadata:0xfedcba9876543210
> +# 0: ff -> (none)
> +# 1: ff -> (none)
> +# 2: 00 -> (none)
> +# 3: 20 -> (none)
> +# 4: 00 -> (none)
> +# 5: 00 -> (none)
> +# 6: 23 -> (none)
> +# 7: 20 -> (none)
> +# 8: 00 -> (none)
> +# 9: 15 -> (none)
> +# 10: 00 -> (none)
> +# 11: 00 -> (none)
> +# 12: 00 -> (none)
> +# 13: 00 -> (none)
> +# 14: 00 -> (none)
> +# 15: 00 -> (none)
> +# 16: fe -> (none)
> +# 17: dc -> (none)
> +# 18: ba -> (none)
> +# 19: 98 -> (none)
> +# 20: 76 -> (none)
> +# 21: 54 -> (none)
> +# 22: 32 -> (none)
> +# 23: 10 -> (none)
> +# 24: ff -> (none)
> +# 25: ff -> (none)
> +# 26: ff -> (none)
> +# 27: ff -> (none)
> +# 28: ff -> (none)
> +# 29: ff -> (none)
> +# 30: ff -> (none)
> +# 31: ff -> (none)
> +ffff 0020 00002320 0015 000000000000 fedcba9876543210 ffffffffffffffff
I'm not sure I understand the above. It seems counterintuitive at
first glance. I need to take another look when I've got a few more
minutes.
> diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
> index 22fc530..2254fdb 100644
> --- a/utilities/ovs-ofctl.8.in
> +++ b/utilities/ovs-ofctl.8.in
> @@ -614,6 +614,14 @@ When this field is specified in \fBadd-flow\fR,
> \fBadd-flows\fR,
> extension to OpenFlow, which as of this writing is only known to be
> implemented by Open vSwitch.
> .
> +.IP \fBmetadata=\fIvalue\fR[\fB/\fImask\fR]
> +Matches \fIvalue\fR either exactly or with optional \fImask\fR in the
> metadata
> +field. \fIvalue\fR and \fImask\fR are 64-bit integers, by default in decimal
> +(use a \fB0x\fR prefix to specify hexadecimal). Arbitrary \fImask\fR values
> +are allowed: a 1-bit in \fImask\fR indicates that the corresponding bit in
> +\fIvalue\fR must match exactly, and a 0-bit wildcards that bit. Matching on
> +metadata was added in Open vSwitch 1.8.
> +.
It looks like the above hunk is really a documentation bug fix for
what's already on master. If so, would you mind submitting it as a
separate patch so I can apply it immediately?
Thanks,
Ben.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
