On Wed, Jul 25, 2012 at 03:06:17PM +0900, Simon Horman wrote:
> On Tue, Jul 24, 2012 at 10:55:17PM -0700, Ben Pfaff wrote:
> > On Mon, Jul 23, 2012 at 03:16:49PM +0900, Simon Horman wrote:
> > > Signed-off-by: Simon Horman <ho...@verge.net.au>
> >
> > It seems reasonable but the reason for:
> >
> > > * Only use OFPERR_NXBRC_BAD_IN_PORT in the case of
> > > OFPRAW_OFPT10_PACKET_OUT
> >
> > isn't obvious to me, can you explain?
>
> My assumption was that is the only context in which that error is valid.
> If not we can just use it everywhere as previous versions of the patch did.
Sorry, my comment didn't make a lot of sense.
The reason for using OFPERR_NXBRC_BAD_IN_PORT in the case of
OFPRAW_OFPT10_PACKET_OUT is that it reflects the exiting behaviour.
assert(raw == OFPRAW_OFPT10_PACKET_OUT);
...
return OFPERR_NXBRC_BAD_IN_PORT;
And I wanted to allow a subsequent patch to use OFPERR_OFPBMC_BAD_VALUE
in the case of Open Flow 1.1 and 1.2, as I felt that using
OFPERR_NXBRC_BAD_IN_PORT would not be valid in those cases. (I notice
the subsequent patch actually uses OFPERR_NXBRC_BAD_IN_PORT, I will fix
that pending advice from you on what the best error to use is.)
Perhaps it would be best to change the behaviour to return
OFPERR_OFPET_BAD_REQUEST in the case of OFPRAW_OFPT10_PACKET_OUT?
For reference the version of the function in question that is currently
in your of1.1 branch is as follows.
/* Converts an OFPT_PACKET_OUT in 'opo' into an abstract ofputil_packet_out in
* 'po'.
*
* Uses 'ofpacts' to store the abstract OFPACT_* version of the packet out
* message's actions. The caller must initialize 'ofpacts' and retains
* ownership of it. 'po->ofpacts' will point into the 'ofpacts' buffer.
*
* Returns 0 if successful, otherwise an OFPERR_* value. */
enum ofperr
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 (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,
po->in_port);
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;
if (po->buffer_id == UINT32_MAX) {
po->packet = b.data;
po->packet_len = b.size;
} else {
po->packet = NULL;
po->packet_len = 0;
}
return 0;
}
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
