Sounds good.
Ethan
On Mon, Feb 6, 2012 at 14:17, Ben Pfaff <b...@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.
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
