[ovs-dev] [PATCH v4 03/14] Implement new packet-in format NXT_PACKET_IN2.

[Ben Pfaff]

On Fri, Feb 19, 2016 at 02:17:18PM -0800, Jarno Rajahalme wrote:
> Acked-by: Jarno Rajahalme <jarno at 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!