On Mon, Dec 01, 2014 at 04:38:51PM +0000, Sharo, Randall A CIV
SPAWARSYSCEN-ATLANTIC, 55200 wrote:
> This patch adds set-field operations for nd_target, nd_sll, and nd_tll
> fields, with and without masks, using Nicira extensions and OpenFlow 1.2
> protocol. Unit tests are included.
>
> This version performs all ND set-field processing in userspace. All ND
> set-field functions pass functional testing, including ICMPv6 checksum
> validation and regression test with IPv4/ARP traffic. Tests were performed
> on CentOS 6.6, CentOS 7, and Ubuntu 14.04 LTS.
>
> This version was developed against the master branch. (Thanks for the
> pointer, Ben!)
>
>
>
> Signed-off-by: Randall A Sharo <randall.sharo at navy.mil>
Thanks for v3! I have some further comments.
Clang reports the following:
../lib/packets.c:884:38: error: cast from 'uint8_t *' (aka 'unsigned char
*') to
'struct nd_neighbor_solicit *' increases required alignment from 1 to
4
[-Werror,-Wcast-align]
struct nd_neighbor_solicit *ns = (struct nd_neighbor_solicit *)option;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
As for the solution to that problem: usually, our strategy for handling
alignment is to define new versions of network protocol structures that
only require 16-bit alignment. You can see examples as
ovs_16aligned_ip6_frag and ovs_16aligned_ip6_hdr. And then when you do
that, you don't need to cast &ns->nd_ns_target to ovs_16aligned_be32 *,
since the new version would have the appropriate type to begin with.
"sparse" reports the following:
../lib/packets.c:910:36: warning: incorrect type in initializer (different
base types)
../lib/packets.c:910:36: expected restricted ovs_be16 [usertype] *csum
../lib/packets.c:910:36: got unsigned short *<noident>
../lib/packets.c:920:36: warning: incorrect type in initializer (different
base types)
../lib/packets.c:920:36: expected restricted ovs_be16 [usertype] *csum
../lib/packets.c:920:36: got unsigned short *<noident>
Reading the code, I see that it uses more casts than we typically do in
OVS. I'll give some examples of what I mean. In recalc_csum48(), the
casts can be removed entirely, e.g.:
const uint16_t *p16_old = (const uint16_t *)old_bytes,
*p16_new = (const uint16_t *)new_bytes;
becomes:
const uint16_t *p16_old = old_bytes;
const uint16_t *p16_new = new_bytes;
Similarly for the first cast in odp_set_nd():
const struct nd_neighbor_solicit *ns
= (struct nd_neighbor_solicit *)ofpbuf_l4(packet);
to just:
const struct nd_neighbor_solicit *ns = ofpbuf_l4(packet);
Also in that function, the later cast of "option" when initializing
nd_opt could be eliminated by changing the type of "option" to const
void *.
It looks like none of the added tests actually demonstrates that the
code correctly changes a packet. It would nice to do that to, e.g.,
increase the confidence in updating the checksums.
Thanks,
Ben.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
