On Fri, Feb 03, 2012 at 12:43:50PM -0800, Ben Pfaff wrote:
> OpenFlow actions have always been somewhat awkward to handle.
> Moreover, over time we've started creating actions that require more
> complicated parsing. When we maintain those actions internally in
> their wire format, we end up parsing them multiple times, whenever
> we have to look at the set of actions.
>
> When we add support for OpenFlow 1.1 or later protocols, the situation
> will get worse, because these newer protocols support many of the same
> actions but with different representations. It becomes unrealistic to
> handle each protocol in its wire format.
>
> This commit adopts a new strategy, by converting OpenFlow actions into
> an internal form from the wire format when they are read, and converting
> them back to the wire format when flows are dumped. I believe that this
> will be more maintainable over time.
>
> The following improvements are necessary before this commit is ready:
>
> - The current commit formats internal actions as Netlink. This is
> an imperfect choice because Netlink attributes are only 4-byte
> aligned, which makes it awkward to include 64-bit integers and
> pointers inside actions. It would be nice to figure out a way to
> do this, either by adding a way to 64-bit align Netlink attributes
> or to use a different framing format.
>
> - Add comments in various places.
>
> - The new functions in netlink, meta-flow, and ofp-util might be easier
> to review as separate commits.
>
> - A few minor points marked with XXX.
[snip]
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index f23b0dd..3b7eac1 100644
[snip]
> @@ -1953,17 +1944,14 @@ reject_slave_controller(struct ofconn *ofconn)
> }
>
> static enum ofperr
> -handle_packet_out(struct ofconn *ofconn, const struct ofp_header *oh)
> +handle_packet_out(struct ofconn *ofconn, const struct ofp_packet_out *opo)
> {
> struct ofproto *p = ofconn_get_ofproto(ofconn);
> - struct ofp_packet_out *opo;
> - struct ofpbuf payload, *buffer;
> - union ofp_action *ofp_actions;
> - struct ofpbuf request;
> + struct ofputil_packet_out po;
> + struct ofpbuf *payload;
> + struct nlattr *ofpacts;
> struct flow flow;
> - size_t n_ofp_actions;
> enum ofperr error;
> - uint16_t in_port;
>
> COVERAGE_INC(ofproto_packet_out);
>
> @@ -1972,28 +1960,21 @@ handle_packet_out(struct ofconn *ofconn, const struct
> ofp_header *oh)
> return error;
> }
>
> - /* Get ofp_packet_out. */
> - ofpbuf_use_const(&request, oh, ntohs(oh->length));
> - opo = ofpbuf_pull(&request, offsetof(struct ofp_packet_out, actions));
> -
> - /* Get actions. */
> - error = ofputil_pull_actions(&request, ntohs(opo->actions_len),
> - &ofp_actions, &n_ofp_actions);
> + /* Decode message. */
> + error = ofputil_decode_packet_out(&po, opo);
> if (error) {
> return error;
> }
>
> /* Get payload. */
> - if (opo->buffer_id != htonl(UINT32_MAX)) {
> - error = ofconn_pktbuf_retrieve(ofconn, ntohl(opo->buffer_id),
> - &buffer, NULL);
> - if (error || !buffer) {
> + if (po.buffer_id != UINT32_MAX) {
> + error = ofconn_pktbuf_retrieve(ofconn, po.buffer_id, &payload, NULL);
> + if (error || !payload) {
> return error;
> }
> - payload = *buffer;
> } else {
> - payload = request;
> - buffer = NULL;
> + payload = xmalloc(sizeof *payload);
> + ofpbuf_use_const(payload, po.packet, po.packet_len);
> }
>
> /* Get in_port and partially validate it.
> @@ -2001,16 +1982,15 @@ handle_packet_out(struct ofconn *ofconn, const struct
> ofp_header *oh)
> * We don't know what range of ports the ofproto actually implements, but
> * we do know that only certain reserved ports (numbered OFPP_MAX and
> * above) are valid. */
> - in_port = ntohs(opo->in_port);
> - if (in_port >= OFPP_MAX && in_port != OFPP_LOCAL && in_port !=
> OFPP_NONE) {
> + if (po.in_port >= OFPP_MAX && po.in_port != OFPP_LOCAL
> + && po.in_port != OFPP_NONE) {
> return OFPERR_NXBRC_BAD_IN_PORT;
> }
>
> /* Send out packet. */
> - flow_extract(&payload, 0, 0, in_port, &flow);
> - error = p->ofproto_class->packet_out(p, &payload, &flow,
> - ofp_actions, n_ofp_actions);
> - ofpbuf_delete(buffer);
> + flow_extract(payload, 0, 0, po.in_port, &flow);
> + error = p->ofproto_class->packet_out(p, payload, &flow, ofpacts);
> + ofpbuf_delete(payload);
>
> return error;
> }
ofpacts seems to be use uninitialised.
Is it necessary to some how initialise ofpacts from
po.actions and po.n_actions?
[snip]
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
