On Thu, Dec 29, 2011 at 01:05:38PM -0800, Ethan Jackson wrote:
> The new PACKET_IN format implemented in this patch includes flow
> metadata such as the cookie, table_id, and registers.
>
> Signed-off-by: Ethan Jackson <et...@nicira.com>
I get some compile errors (maybe as a symptom of conflicting changes?
or maybe I applied the wrong version?):
../lib/ofp-util.c: In function 'ofputil_decode_packet_in':
../lib/ofp-util.c:1613: error: too many arguments to function
'nx_pull_match'
../lib/ofp-util.c: In function 'ofputil_encode_packet_in':
../lib/ofp-util.c:1693: error: too many arguments to function 'nx_put_match'
There's a double blank line above the definition of
NXT_SET_PACKET_IN_FORMAT. (Did you intend to put some high-level
comment there?) Also a blank line after the new NXT_PACKET_IN.
It might be reasonable to reuse the NXFF_* values for the 'format'
member in nxt_set_packet_in_format, instead of defining new NXPIF_*
values. Or you could even define a new NXFF_NXM2 that is NXFF_NXM
plus packet-in support, which would allow you to drop the separate
NXT_SET_PACKET_IN_FORMAT message. But the way you have it is also
fine, so don't feel obligated to change anything.
Missing space here:
> +enum nx_packet_in_format{
I'd extend the comment on struct nxt_packet_in to mention a few more
points:
* This format is a slightly extended version of the OpenFlow
1.2 packet-in (with the cookie added).
* The meaning of the nxm_fields (are we including the whole
match or just the metadata? can the controller rely on that
in the future? etc.)
* Whereas in most cases a controller can expect to only get
back NXM fields that it set up itself (e.g. flow dumps will
ordinarily report only NXM fields from flows that the
controller added), packet-ins might contain fields that the
controller does not understand, because the switch might
support fields (new registers, new protocols, etc.) that the
controller does not. The controller must prepared to
tolerate these.
What's the meaning/value of table_id and cookie in struct
nxt_packet_in if the reason is OFPR_MISS?
tun_id is also a metadata field but this patch doesn't export it via
packet_in, why not?
Did you consider adding a struct to encapsulate metadata, e.g. "struct
flow_metadata" or similar?
I don't see why the change to nx-match.h is necessary.
In ofputil_decode_packet_in(), I think we need a more relaxed version
of nx_pull_match() that doesn't report an error just because of NXM
fields that are not understood.
Also in ofputil_decode_packet_in(), the call to ofpbuf_use_const() is
unsafe. We cannot depend on npi->match_len to be correct: it might
overrun the end of the packet_in message. It would be better to
follow the pattern used elsewhere, e.g. in ofputil_decode_flow_mod()
or ofputil_decode_nxst_flow_request(), by putting the entire message
in an ofpbuf, pulling off the nxt_packet_in fixed-size header, then
using nx_pull_match(). After that, pull off 2 more bytes and whatever
is left is the packet data.
Still in ofputil_decode_packet_in(), the use of total_len looks wrong,
too. We can't trust it, to start with, but it's not the length of
anything in the packet anyway (it's the length of the packet before it
was truncated by the switch).
ofputil_encode_packet_in() needs a comment update.
The overall strategy in ofputil_encode_packet_in() now makes less
sense. Previously, the size of the header was fixed, so there was
generally no need to allocate additional headroom. Even if the packet
needed cloning, the clone was created with enough headroom. But now
the required headroom is variable (though bounded by a constant), and
the caller (execute_controller_action(), indirectly) never gives the
packet any headroom anyway. Only one caller remains that actually
transfers ownership to ofputil_encode_packet_in() gives (possibly)
enough headroom to avoid reallocating, which is handle_flow_miss().
My suggestion is:
* Add a macro to represent the maximum amount of headroom
needed by ofputil_encode_packet_in().
* Make execute_controller_action() provide that much headroom.
* Make ofputil_encode_packet_in() allocate that much headroom
when cloning.
s/surpring/surprising/ in a comment in ofproto.c
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
