[ovs-dev] [PATCH 20/24] ofp-util: Prepare Packet Out decoder for other Open Flow versions

Simon Horman horms at verge.net.au
Fri Jul 27 02:29:28 UTC 2012


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 <horms at 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;
}



More information about the dev mailing list