[ovs-dev] [async-msgs 05/13] ofp-util: Add struct ofputil_packet_out, helper functions, and use it all.

[Ethan Jackson]

Sounds good.

Ethan

On Mon, Feb 6, 2012 at 14:17, Ben Pfaff <blp at nicira.com> wrote:

> On Mon, Feb 06, 2012 at 01:59:05PM -0800, Ethan Jackson wrote:
> > >
> > >                              (Only meaningful if buffer_id == -1.) */
> > > +    /* Followed by:
> > > +     *   - Exactly 'actions_len' bytes (possibly 0 bytes, and always a
> > > multiple
> > > +     *     of 8) containing actions.
> > > +     *   - If 'buffer_id' != -1, packet data to fill out the
> remainder of
> > > the
> > > +     *     message length.
> > > +     */
> > >
> >
> > I think you mean "If 'buffer_id' == -1".
>
> Yes, thanks.
>
> I decided to make things even clearer:
>
> @@ -450,14 +450,14 @@ OFP_ASSERT(sizeof(union ofp_action) == 8);
>  /* Send packet (controller -> datapath). */
>  struct ofp_packet_out {
>     struct ofp_header header;
> -    ovs_be32 buffer_id;           /* ID assigned by datapath (-1 if
> none). */
> +    ovs_be32 buffer_id;           /* ID assigned by datapath or
> UINT32_MAX. */
>      ovs_be16 in_port;             /* Packet's input port (OFPP_NONE if
> none). */
>     ovs_be16 actions_len;         /* Size of action array in bytes. */
>      /* Followed by:
>       *   - Exactly 'actions_len' bytes (possibly 0 bytes, and always a
> multiple
>       *     of 8) containing actions.
> -     *   - If 'buffer_id' != -1, packet data to fill out the remainder of
> the
> -     *     message length.
> +     *   - If 'buffer_id' == UINT32_MAX, packet data to fill out the
> remainder
> +     *     of the message length.
>       */
>  };
>  OFP_ASSERT(sizeof(struct ofp_packet_out) == 16);
>
> > > +    if (po->in_port >= OFPP_MAX && po->in_port != OFPP_NONE) {
> > > +        VLOG_WARN_RL(&bad_ofmsg_rl, "packet-out has bad input port
> > > %#"PRIx16,
> > > +                     po->in_port);
> > > +        return OFPERR_NXBRC_BAD_IN_PORT;
> > > +    }
> > >
> >
> > OFPP_LOCAL is also a valid input port.
>
> Good catch, thank you.  I added "&& po->in_port != OFPP_LOCAL", which
> matches what was previously in handle_packet_out().
>
> >
> > > +    if (po.in_port >= OFPP_MAX && po.in_port != OFPP_LOCAL
> > > +        && po.in_port != OFPP_NONE) {
> > >         return OFPERR_NXBRC_BAD_IN_PORT;
> > >     }
> > >
> >
> > This check is now redundant since ofptuil_decode_packet_out() does the
> same
> > validation.
>
> Thank you, I removed this check from here.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20120206/5fa52bb5/attachment-0003.html>