On Fri, Feb 19, 2016 at 02:17:18PM -0800, Jarno Rajahalme wrote:
> Acked-by: Jarno Rajahalme <ja...@ovn.org>
Thanks for the review!
> > + * From OVS v1.1 to OVS v2.5, this request was only honored for OpenFlow
> > 1.0.
> > + * Requests to set format NXPIF_NXT_PACKET_IN were accepted for OF1.1+ but
> > they
> > + * had no effect. (Requests to set formats other than NXPIF_STANDARD or
> > + * NXPIF_NXT_PACKET_IN2 were rejected with OFPBRC_EPERM.)
> > + *
>
> This should be NXPIF_NXT_PACKET_IN
Fixed, thanks.
> > +enum nx_packet_in2_prop_type {
> > + /* Packet. */
> > + NXPINT_PACKET, /* Raw packet data. */
> > + NXPINT_FULL_LEN, /* ovs_be32: Full packet len, if
> > truncated. */
> > + NXPINT_BUFFER_ID, /* ovs_be32: Buffer ID, if buffered. */
> > +
> > + /* Information about the flow that triggered the packet-in. */
> > + NXPINT_TABLE_ID, /* uint8_t: Table ID. */
> > + NXPINT_COOKIE, /* ovs_be64: Flow cookie (all-1s if none).
> > */
>
> Would leaving out the property in the none case work as well? I.e., do
> we need the all-1s case?
It's the same either way.
I removed the parenthetical.
> > --- a/lib/ofp-msgs.h
> > +++ b/lib/ofp-msgs.h
> > @@ -152,6 +152,8 @@ enum ofpraw {
> > OFPRAW_OFPT13_PACKET_IN,
> > /* NXT 1.0+ (17): struct nx_packet_in, uint8_t[]. */
> > OFPRAW_NXT_PACKET_IN,
> > + /* NXT 1.0+ (30): uint8_t[]. */
>
> As properties are 8-byte aligned (?), could this be expressed as
> uint8_t[8][], or as uint64_t[]?
You're right, I changed it to uint8_t[8][].
> > +static enum ofperr
> > +decode_nx_packet_in2(const struct ofp_header *oh,
> > + struct ofputil_packet_in *pin,
> > + size_t *total_len, uint32_t *buffer_id)
>
> Is the caller responsible for initializing ‘*pin’ before making the call?
Well, yes, this is just a helper for ofputil_decode_packet_in(), which
does initialize it.
I added a comment.
> > +{
> > + *total_len = 0;
> > + *buffer_id = UINT32_MAX;
> > +
> > + struct ofpbuf properties;
> > + ofpbuf_use_const(&properties, oh, ntohs(oh->length));
> > + ofpraw_pull_assert(&properties);
> > +
> > + while (properties.size > 0) {
> > + struct ofpbuf payload;
> > + uint64_t type;
> > +
> > + enum ofperr error = ofpprop_pull(&properties, &payload, &type);
> > + if (error) {
> > + return error;
> > + }
> > +
> > + switch (type) {
> > + case NXPINT_PACKET:
> > + pin->packet = payload.msg;
> > + pin->len = ofpbuf_msgsize(&payload);
> > + break;
> > +
> > + case NXPINT_FULL_LEN: {
> > + uint32_t u32;
> > + error = ofpprop_parse_u32(&payload, &u32);
> > + *total_len = u32;
> > + break;
> > + }
> > +
> > + case NXPINT_BUFFER_ID:
> > + error = ofpprop_parse_u32(&payload, buffer_id);
> > + break;
> > +
> > + case NXPINT_TABLE_ID:
> > + error = ofpprop_parse_u8(&payload, &pin->table_id);
> > + break;
> > +
> > + case NXPINT_COOKIE:
> > + error = ofpprop_parse_be64(&payload, &pin->cookie);
> > + break;
>
> We should fill in all-1s if this property is missing?
The caller does that.
> > +
> > + case NXPINT_REASON: {
> > + uint8_t reason;
> > + error = ofpprop_parse_u8(&payload, &reason);
> > + pin->reason = reason;
> > + break;
> > + }
> > +
> > + case NXPINT_METADATA:
> > + error = oxm_decode_match(payload.msg, ofpbuf_msgsize(&payload),
> > + &pin->flow_metadata);
> > + break;
> > +
> > + default:
> > + error = OFPPROP_UNKNOWN(false, "NX_PACKET_IN2", type);
> > + break;
>
> Could unknown properties be silently ignored? Or should we define a
> range of properties that could be silently ignored if unknown?
Hmm, I meant to ignore all unknown properties here but I got the 'loose'
argument to OFPPROP_UNKNOWN wrong. I flipped it to true. Thanks.
My rationale is that the controller is just providing the switch
information here, and it's harmless if it provides extra information
(extra properties).
> > /* Decodes the packet-in message starting at 'oh' into '*pin'. Populates
> > * 'pin->packet' and 'pin->len' with the part of the packet actually
> > included
> > * in the message, and '*total_len' with the original length of the packet
> > @@ -3394,6 +3481,8 @@ ofputil_decode_packet_in(const struct ofp_header *oh,
> >
> > pin->packet = b.data;
> > pin->len = b.size;
> > + } else if (raw == OFPRAW_NXT_PACKET_IN2) {
> > + return decode_nx_packet_in2(oh, pin, total_len, buffer_id);
> > } else {
> > OVS_NOT_REACHED();
> > }
> > @@ -3448,14 +3537,14 @@ ofputil_encode_ofp10_packet_in(const struct
> > ofputil_packet_in *pin,
> >
> > static struct ofpbuf *
> > ofputil_encode_nx_packet_in(const struct ofputil_packet_in *pin,
> > - uint32_t buffer_id)
> > + enum ofp_version version, uint32_t buffer_id)
>
> indentation wrong here?
Fixed, thanks!
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
