Re: [ovs-dev] [PATCH 09/41] ofp-util: Prepare Packet Out decoder for other Open Flow versions

2012-08-08 Thread Simon Horman
On Wed, Aug 08, 2012 at 05:00:02PM -0700, Ben Pfaff wrote:
> On Thu, Aug 09, 2012 at 08:56:10AM +0900, Simon Horman wrote:
> > On Wed, Aug 08, 2012 at 04:42:30PM -0700, Ben Pfaff wrote:
> > > On Wed, Aug 08, 2012 at 06:49:44AM +0900, Simon Horman wrote:
> > > > Signed-off-by: Simon Horman 
> > > >  {
> > > > -const struct ofp_packet_out *opo;
> > > > -enum ofperr error;
> > > >  enum ofpraw raw;
> > > >  struct ofpbuf b;
> > > > +enum ofperr bad_in_port_err = OFPERR_OFPET_BAD_REQUEST;
> > > >  
> > > >  ofpbuf_use_const(&b, oh, ntohs(oh->length));
> > > >  raw = ofpraw_pull_assert(&b);
> > > > -assert(raw == OFPRAW_OFPT10_PACKET_OUT);
> > > >  
> > > > -opo = ofpbuf_pull(&b, sizeof *opo);
> > > > -po->buffer_id = ntohl(opo->buffer_id);
> > > > -po->in_port = ntohs(opo->in_port);
> > > > +if (raw == OFPRAW_OFPT10_PACKET_OUT) {
> > > > +enum ofperr error;
> > > > +const struct ofp_packet_out *opo = ofpbuf_pull(&b, sizeof 
> > > > *opo);
> > > > +
> > > > +po->buffer_id = ntohl(opo->buffer_id);
> > > > +po->in_port = ntohs(opo->in_port);
> > > > +
> > > > +error = ofpacts_pull_openflow10(&b, ntohs(opo->actions_len), 
> > > > ofpacts);
> > > > +if (error) {
> > > > +return error;
> > > > +}
> > > > +bad_in_port_err = OFPERR_NXBRC_BAD_IN_PORT;
> > > > +} else {
> > > > +NOT_REACHED();
> > > > +}
> > > 
> > > I think that we should consistently use OFPERR_NXBRC_BAD_IN_PORT.
> > > OFPERR_OFPET_BAD_REQUEST isn't a good substitute because it isn't
> > > encodable as an error at all (it is an error category, not an error
> > > itself).
> > > 
> > > If there's a better choice for a standard error code in some OpenFlow
> > > version then I'm happy to use that.
> > 
> > I'm comfortable with using OFPERR_NXBRC_BAD_IN_PORT if you are.
> 
> Thanks.
> 
> This is what I ended up with:

Thanks, looks good.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev


Re: [ovs-dev] [PATCH 09/41] ofp-util: Prepare Packet Out decoder for other Open Flow versions

2012-08-08 Thread Ben Pfaff
On Thu, Aug 09, 2012 at 08:56:10AM +0900, Simon Horman wrote:
> On Wed, Aug 08, 2012 at 04:42:30PM -0700, Ben Pfaff wrote:
> > On Wed, Aug 08, 2012 at 06:49:44AM +0900, Simon Horman wrote:
> > > Signed-off-by: Simon Horman 
> > >  {
> > > -const struct ofp_packet_out *opo;
> > > -enum ofperr error;
> > >  enum ofpraw raw;
> > >  struct ofpbuf b;
> > > +enum ofperr bad_in_port_err = OFPERR_OFPET_BAD_REQUEST;
> > >  
> > >  ofpbuf_use_const(&b, oh, ntohs(oh->length));
> > >  raw = ofpraw_pull_assert(&b);
> > > -assert(raw == OFPRAW_OFPT10_PACKET_OUT);
> > >  
> > > -opo = ofpbuf_pull(&b, sizeof *opo);
> > > -po->buffer_id = ntohl(opo->buffer_id);
> > > -po->in_port = ntohs(opo->in_port);
> > > +if (raw == OFPRAW_OFPT10_PACKET_OUT) {
> > > +enum ofperr error;
> > > +const struct ofp_packet_out *opo = ofpbuf_pull(&b, sizeof *opo);
> > > +
> > > +po->buffer_id = ntohl(opo->buffer_id);
> > > +po->in_port = ntohs(opo->in_port);
> > > +
> > > +error = ofpacts_pull_openflow10(&b, ntohs(opo->actions_len), 
> > > ofpacts);
> > > +if (error) {
> > > +return error;
> > > +}
> > > +bad_in_port_err = OFPERR_NXBRC_BAD_IN_PORT;
> > > +} else {
> > > +NOT_REACHED();
> > > +}
> > 
> > I think that we should consistently use OFPERR_NXBRC_BAD_IN_PORT.
> > OFPERR_OFPET_BAD_REQUEST isn't a good substitute because it isn't
> > encodable as an error at all (it is an error category, not an error
> > itself).
> > 
> > If there's a better choice for a standard error code in some OpenFlow
> > version then I'm happy to use that.
> 
> I'm comfortable with using OFPERR_NXBRC_BAD_IN_PORT if you are.

Thanks.

This is what I ended up with:

--8<--cut here-->8--

From: Simon Horman 
Date: Wed, 8 Aug 2012 06:49:44 +0900
Subject: [PATCH] ofp-util: Prepare Packet Out decoder for other Open Flow 
versions

Signed-off-by: Simon Horman 
Signed-off-by: Ben Pfaff 
---
 lib/ofp-util.c |   25 +++--
 1 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 4632482..64eb5ca 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -2149,18 +2149,27 @@ ofputil_decode_packet_out(struct ofputil_packet_out *po,
   const struct ofp_header *oh,
   struct ofpbuf *ofpacts)
 {
-const struct ofp_packet_out *opo;
-enum ofperr error;
 enum ofpraw raw;
 struct ofpbuf b;
 
 ofpbuf_use_const(&b, oh, ntohs(oh->length));
 raw = ofpraw_pull_assert(&b);
-assert(raw == OFPRAW_OFPT10_PACKET_OUT);
 
-opo = ofpbuf_pull(&b, sizeof *opo);
-po->buffer_id = ntohl(opo->buffer_id);
-po->in_port = ntohs(opo->in_port);
+if (raw == OFPRAW_OFPT10_PACKET_OUT) {
+enum ofperr error;
+const struct ofp_packet_out *opo = ofpbuf_pull(&b, sizeof *opo);
+
+po->buffer_id = ntohl(opo->buffer_id);
+po->in_port = ntohs(opo->in_port);
+
+error = ofpacts_pull_openflow10(&b, ntohs(opo->actions_len), ofpacts);
+if (error) {
+return error;
+}
+} else {
+NOT_REACHED();
+}
+
 if (po->in_port >= OFPP_MAX && po->in_port != OFPP_LOCAL
 && po->in_port != OFPP_NONE && po->in_port != OFPP_CONTROLLER) {
 VLOG_WARN_RL(&bad_ofmsg_rl, "packet-out has bad input port %#"PRIx16,
@@ -2168,10 +2177,6 @@ ofputil_decode_packet_out(struct ofputil_packet_out *po,
 return OFPERR_NXBRC_BAD_IN_PORT;
 }
 
-error = ofpacts_pull_openflow10(&b, ntohs(opo->actions_len), ofpacts);
-if (error) {
-return error;
-}
 po->ofpacts = ofpacts->data;
 po->ofpacts_len = ofpacts->size;
 
-- 
1.7.2.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev


Re: [ovs-dev] [PATCH 09/41] ofp-util: Prepare Packet Out decoder for other Open Flow versions

2012-08-08 Thread Simon Horman
On Wed, Aug 08, 2012 at 04:42:30PM -0700, Ben Pfaff wrote:
> On Wed, Aug 08, 2012 at 06:49:44AM +0900, Simon Horman wrote:
> > Signed-off-by: Simon Horman 
> >  {
> > -const struct ofp_packet_out *opo;
> > -enum ofperr error;
> >  enum ofpraw raw;
> >  struct ofpbuf b;
> > +enum ofperr bad_in_port_err = OFPERR_OFPET_BAD_REQUEST;
> >  
> >  ofpbuf_use_const(&b, oh, ntohs(oh->length));
> >  raw = ofpraw_pull_assert(&b);
> > -assert(raw == OFPRAW_OFPT10_PACKET_OUT);
> >  
> > -opo = ofpbuf_pull(&b, sizeof *opo);
> > -po->buffer_id = ntohl(opo->buffer_id);
> > -po->in_port = ntohs(opo->in_port);
> > +if (raw == OFPRAW_OFPT10_PACKET_OUT) {
> > +enum ofperr error;
> > +const struct ofp_packet_out *opo = ofpbuf_pull(&b, sizeof *opo);
> > +
> > +po->buffer_id = ntohl(opo->buffer_id);
> > +po->in_port = ntohs(opo->in_port);
> > +
> > +error = ofpacts_pull_openflow10(&b, ntohs(opo->actions_len), 
> > ofpacts);
> > +if (error) {
> > +return error;
> > +}
> > +bad_in_port_err = OFPERR_NXBRC_BAD_IN_PORT;
> > +} else {
> > +NOT_REACHED();
> > +}
> 
> I think that we should consistently use OFPERR_NXBRC_BAD_IN_PORT.
> OFPERR_OFPET_BAD_REQUEST isn't a good substitute because it isn't
> encodable as an error at all (it is an error category, not an error
> itself).
> 
> If there's a better choice for a standard error code in some OpenFlow
> version then I'm happy to use that.

I'm comfortable with using OFPERR_NXBRC_BAD_IN_PORT if you are.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev


Re: [ovs-dev] [PATCH 09/41] ofp-util: Prepare Packet Out decoder for other Open Flow versions

2012-08-08 Thread Ben Pfaff
On Wed, Aug 08, 2012 at 06:49:44AM +0900, Simon Horman wrote:
> Signed-off-by: Simon Horman 
>  {
> -const struct ofp_packet_out *opo;
> -enum ofperr error;
>  enum ofpraw raw;
>  struct ofpbuf b;
> +enum ofperr bad_in_port_err = OFPERR_OFPET_BAD_REQUEST;
>  
>  ofpbuf_use_const(&b, oh, ntohs(oh->length));
>  raw = ofpraw_pull_assert(&b);
> -assert(raw == OFPRAW_OFPT10_PACKET_OUT);
>  
> -opo = ofpbuf_pull(&b, sizeof *opo);
> -po->buffer_id = ntohl(opo->buffer_id);
> -po->in_port = ntohs(opo->in_port);
> +if (raw == OFPRAW_OFPT10_PACKET_OUT) {
> +enum ofperr error;
> +const struct ofp_packet_out *opo = ofpbuf_pull(&b, sizeof *opo);
> +
> +po->buffer_id = ntohl(opo->buffer_id);
> +po->in_port = ntohs(opo->in_port);
> +
> +error = ofpacts_pull_openflow10(&b, ntohs(opo->actions_len), 
> ofpacts);
> +if (error) {
> +return error;
> +}
> +bad_in_port_err = OFPERR_NXBRC_BAD_IN_PORT;
> +} else {
> +NOT_REACHED();
> +}

I think that we should consistently use OFPERR_NXBRC_BAD_IN_PORT.
OFPERR_OFPET_BAD_REQUEST isn't a good substitute because it isn't
encodable as an error at all (it is an error category, not an error
itself).

If there's a better choice for a standard error code in some OpenFlow
version then I'm happy to use that.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev