[ovs-dev] [PATCH 1/3] auto-attach: Initial support for Auto-Attach standard

[Ben Pfaff]

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.beliveau at windriver.com>
> Signed-off-by: Dennis Flynn <drflynn at 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.