Hi Ben, Thank you for the thorough review of the auto attach patches!
We will address all your comments/concerns and be back in touch. Thanks again, Dennis -----Original Message----- From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Ben Pfaff Sent: Monday, October 13, 2014 6:49 PM To: Ludovic Beliveau Cc: dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH 1/3] auto-attach: Initial support for Auto-Attach standard On Thu, Oct 02, 2014 at 06:42:24PM -0400, Ludovic Beliveau wrote: > This commit provides the initial delivery of support for the > Auto-Attach standard to Open vSwitch. This standard describes a > compact method of using IEEE 802.1AB Link Layer Discovery Protocol > (LLDP) with a IEEE 802.1aq Shortest Path Bridging (SPB) network to > automatically attach network devices not supporting IEEE 802.1ah to > individual services in a SPB network. Specifically this commit adds > base LLDP support to OVS along with the LLDP extension required to support > Auto-Attach. > > The base LLDP code within this commit is adapted from the open source > LLDPD project headed by Vincent Bernat. This base code is augmented > with OVS specific logic which integrates LLDP into OVS and which > extends LLDP to support Auto-Attach. The required build system changes > are included to configure and build OVS with this new feature. > > This is the first of a series of commits. Subsequent commits will be > provided to complete the task of adding Auto-Attach to OVS. > > Signed-off-by: Ludovic Beliveau <ludovic.beliv...@windriver.com> > Signed-off-by: Dennis Flynn <drfl...@avaya.com> Thanks for the patch! I have some comments. GCC reports: ../lib/lldp.c: In function 'lldp_send': ../lib/lldp.c:515:9: error: assignment from incompatible pointer type [-Werror] ../lib/lldp.c: In function 'lldp_decode': ../lib/lldp.c:1192:25: error: assignment from incompatible pointer type [-Werror] ../lib/lldp.c:1192:25: error: assignment from incompatible pointer type [-Werror] ../lib/lldp.c:1192:25: error: assignment from incompatible pointer type [-Werror] ../lib/ovs-lldp.c: In function 'aa_print_isid_status': ../lib/ovs-lldp.c:317:17: error: assignment from incompatible pointer type [-Werror] cc1: all warnings being treated as errors ../lib/ovs-lldp.c: In function 'update_mapping_on_lldp': ../lib/ovs-lldp.c:433:5: error: assignment from incompatible pointer type [-Werror] ../lib/ovs-lldp.c:433:5: error: assignment from incompatible pointer type [-Werror] ../lib/ovs-lldp.c:433:5: error: assignment from incompatible pointer type [-Werror] ../lib/ovs-lldp.c: In function 'aa_mapping_unregister': ../lib/ovs-lldp.c:659:17: error: assignment from incompatible pointer type [-Werror] ../lib/ovs-lldp.c:669:25: error: assignment from incompatible pointer type [-Werror] "sparse" reports: ../lib/lldp-frame.c:65:17: warning: incorrect type in argument 1 (different base types) ../lib/lldp-frame.c:65:17: expected restricted ovs_be16 [usertype] x ../lib/lldp-frame.c:65:17: got unsigned int [unsigned] [assigned] sum ../lib/lldpd.c:517:36: warning: restricted ovs_be16 degrades to integer ../lib/lldpd.c:525:5: warning: incorrect type in argument 1 (different base types) ../lib/lldpd.c:525:5: expected restricted ovs_be16 [usertype] x ../lib/lldpd.c:525:5: got unsigned short [unsigned] [addressable] [usertype] ether_type There's a significant amount of duplication here between data structures already implemented in OVS and data structures implemented in this patch. Open vSwitch already has a doubly linked list type, for example, but this patch imports and uses the linked list types and macros from BSD. I'd prefer to avoid this kind of duplication. Usually, bit-fields are not a portable way to represent wire formats, at least without compile-time conditionals for endianness, because different compilers and architectures lay them out differently. I see some use of bit-fields in this patch, e.g. in aa-structs.h, without tests for endianness. Do these structures represent protocol wire formats? I see that support for auto-attach can be enabled and disabled at configuration time. This is unusual for an OVS feature: usually, we build in support for every feature. This makes testing and maintenance easier because it reduces the number of configurations that must be built and tested. It also reduces the number of #ifdefs in the source code (I see many #ifdefs for auto-attach). Is there some reason that auto-attach cannot be built unconditionally? Actually, I wonder whether this has been tested when configuring without --enable-auto-attach. This code in lib/marshal.h indicates that it would fail to build without --enable-auto-attach: #ifdef ENABLE_AUTO_ATTACH ignorem, // ignore has a direct naming conflict in ovs (and therefore has been renamed) #else ignore #endif This patch adds many files with various licenses and copyright holders, so it should update NOTICE at the top level and copyright.in in the debian directory with new details. On that same note, lldp-frame.h says: /* This set of macro are used to build packets. The current position in buffer * is `pos'. The length of the remaining space in buffer is `length'. `type' * should be a member of `types'. This was stolen from ladvd which was adapted * from Net::CDP. */ What are the licenses for ladvd and Net::CDP? One would hope that they are the same as the license for lldp-frame.h, or compatible with that license, but one must check. There are several instances of "#ifdef HOST_OS_LINUX", but I don't see anything that would defined HOST_OS_LINUX. There seem to be other assumptions that the code is running on a Linux kernel (e.g. reference to sysfs), but Linux is only one target for OVS. There's a lot of tests for other macros that aren't going to be defined, such as ENABLE_CDP, ENABLE_FDP, ENABLE_SONMP, ENABLE_EDP. Some of the header files #include <config.h>. They should not; only .c files should #include <config.h>, as their first non-comment line. I think a lot of the above comes down to a question of whether the imported lldpd code is going to become part of OVS, or whether it is going to remain part of lldpd and only used by OVS. If it is the former, then it should adapt to fit Open vSwitch styles and conventions. If it is the latter, then it is probably better to modify it as little as possible so that it can be updated from lldpd later. Ideally, in that case, the lldpd code would not be modified at all and OVS would link against it as an external library. I don't understand the following change. Does it fix some bug? diff --git a/lib/packets.c b/lib/packets.c index 65d8109..928a9e6 100644 --- a/lib/packets.c +++ b/lib/packets.c @@ -575,6 +575,7 @@ eth_compose(struct ofpbuf *b, const uint8_t eth_dst[ETH_ADDR eth = ofpbuf_put_uninit(b, ETH_HEADER_LEN); data = ofpbuf_put_uninit(b, size); + memset(ofpbuf_base(b), 0, size); memcpy(eth->eth_dst, eth_dst, ETH_ADDR_LEN); memcpy(eth->eth_src, eth_src, ETH_ADDR_LEN); eth->eth_type = htons(eth_type); Some of the code here seems to assume GCC (or Clang) as compiler, e.g. lib/marshal.h uses GCC's __attribute__ extension without a test for GCC. OVS also needs to compile with MSVC 2013. However OVS doesn't support GCC 3.x or earlier, so the check for that in lib/marshal.h may be removed. The code in lldp-frame.c just calculates a checksum, so it would fit better into csum.c. I think it can reuse some of the existing csum functions, also. Some of this code #includes a "log.h" that is not part of the tree. It should use the OVS vlog.h instead. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
