On Sat, Jan 07, 2012 at 11:11:59AM -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 <[email protected]>
I don't think that we need to pad nxt_set_packet_in_format to a
multiple of 8 bytes.
s/OpenFLow/OpenFlow/ here:
> +/* NXT_PACKET_IN (analogous to OFPT_PACKET_IN).
> + *
> + * The NXT_PACKET_IN format is intended to model the OpenFLow-1.2 PACKET_IN
Please add a test for NXT_SET_PACKET_IN_FORMAT and one for
NXT_PACKET_IN to tests/ofp-print.at.
I don't think the statement ofpbuf_pull(&b, 2); in
ofputil_decode_packet_in() is safe. I think that it could
assert-fail. It would be better to use ofpbuf_try_pull() and return
an error on failure.
In ofputil_decode_packet_in(), I think that the following fails to
byte-swap in_port or the registers and their masks:
> + pin->fmd.in_port = rule.flow.in_port;
> +
> + pin->fmd.tun_id = rule.flow.tun_id;
> + pin->fmd.tun_id_mask = rule.wc.tun_id_mask;
> +
> + memcpy(pin->fmd.regs, rule.flow.regs, sizeof pin->fmd.regs);
> + memcpy(pin->fmd.reg_masks, rule.wc.reg_masks,
> + sizeof pin->fmd.reg_masks);
In ofputil_encode_packet_in():
- It looks odd to me to initially allocate a 0-byte buffer
then immediately use ofpbuf_prealloc_tailroom(). I think it
would be better to just call ofpbuf_new() with the correct
(estimated) length in each case.
- rw_packet is now an odd name for the ofpbuf.
- In two places, total_len is initialized from
pin->packet_len, but should be initialized from
pin->total_len.
In ovs-ofctl, I think it would be better to follow the pattern that we
have for e.g. "dump-flows", where by default we try to use the Nicira
extension but fall back to the OpenFlow 1.0 version if it is
unavailable, and allow command-line options to force some particular
version.
In ovs-ofctl.8, in the documentation for --packet-in-format, I think
we should mention that it affects only the "monitor" and "snoop"
commands.
Thanks,
Ben.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
