On Fri, Nov 14, 2014 at 07:42:07AM -0500, drfl...@avaya.com wrote:
> From: Dennis Flynn <drfl...@avaya.com>
>
> 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 provided to include this
> new
> Auto-Attach 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>
Sorry for the long delay in review.
lldp.c and lldpd.c provoke a lot of warnings from "sparse":
../lib/lldp/lldpd.c:79:12: warning: Using plain integer as NULL pointer
../lib/lldp/lldpd.c:214:39: warning: Using plain integer as NULL pointer
../lib/lldp/lldpd.c:214:59: warning: Using plain integer as NULL pointer
../lib/lldp/lldpd.c:214:80: warning: Using plain integer as NULL pointer
../lib/lldp/lldp.c:183:11: warning: incorrect type in assignment (different
base types)
../lib/lldp/lldp.c:183:11: expected unsigned short static [unsigned]
[toplevel] [usertype] f_uint16
../lib/lldp/lldp.c:183:11: got restricted ovs_be16
../lib/lldp/lldp.c:186:11: warning: invalid assignment: |=
../lib/lldp/lldp.c:186:11: left side has type unsigned short
../lib/lldp/lldp.c:186:11: right side has type restricted ovs_be16
../lib/lldp/lldp.c:191:11: warning: incorrect type in assignment (different
base types)
../lib/lldp/lldp.c:191:11: expected unsigned short static [unsigned]
[toplevel] [assigned] [usertype] f_uint16
../lib/lldp/lldp.c:191:11: got restricted ovs_be16
../lib/lldp/lldp.c:194:11: warning: invalid assignment: |=
../lib/lldp/lldp.c:194:11: left side has type unsigned short
../lib/lldp/lldp.c:194:11: right side has type restricted ovs_be16
../lib/lldp/lldp.c:199:11: warning: incorrect type in assignment (different
base types)
../lib/lldp/lldp.c:199:11: expected unsigned short static [unsigned]
[toplevel] [assigned] [usertype] f_uint16
../lib/lldp/lldp.c:199:11: got restricted ovs_be16
../lib/lldp/lldp.c:200:11: warning: incorrect type in assignment (different
base types)
../lib/lldp/lldp.c:200:11: expected unsigned short static [unsigned]
[toplevel] [assigned] [usertype] f_uint16
../lib/lldp/lldp.c:200:11: got restricted ovs_be16
../lib/lldp/lldp.c:201:11: warning: invalid assignment: |=
../lib/lldp/lldp.c:201:11: left side has type unsigned short
../lib/lldp/lldp.c:201:11: right side has type restricted ovs_be16
../lib/lldp/lldp.c:207:15: warning: incorrect type in assignment (different
base types)
../lib/lldp/lldp.c:207:15: expected unsigned short static [unsigned]
[toplevel] [assigned] [usertype] f_uint16
../lib/lldp/lldp.c:207:15: got restricted ovs_be16
../lib/lldp/lldp.c:209:15: warning: invalid assignment: |=
../lib/lldp/lldp.c:209:15: left side has type unsigned short
../lib/lldp/lldp.c:209:15: right side has type restricted ovs_be16
../lib/lldp/lldp.c:216:15: warning: incorrect type in assignment (different
base types)
../lib/lldp/lldp.c:216:15: expected unsigned short static [unsigned]
[toplevel] [assigned] [usertype] f_uint16
../lib/lldp/lldp.c:216:15: got restricted ovs_be16
../lib/lldp/lldp.c:218:15: warning: invalid assignment: |=
../lib/lldp/lldp.c:218:15: left side has type unsigned short
../lib/lldp/lldp.c:218:15: right side has type restricted ovs_be16
../lib/lldp/lldp.c:224:11: warning: incorrect type in assignment (different
base types)
../lib/lldp/lldp.c:224:11: expected unsigned short static [unsigned]
[toplevel] [assigned] [usertype] f_uint16
../lib/lldp/lldp.c:224:11: got restricted ovs_be16
../lib/lldp/lldp.c:225:11: warning: incorrect type in assignment (different
base types)
../lib/lldp/lldp.c:225:11: expected unsigned short static [unsigned]
[toplevel] [assigned] [usertype] f_uint16
../lib/lldp/lldp.c:225:11: got restricted ovs_be16
../lib/lldp/lldp.c:226:11: warning: incorrect type in assignment (different
base types)
../lib/lldp/lldp.c:226:11: expected unsigned short static [unsigned]
[toplevel] [assigned] [usertype] f_uint16
../lib/lldp/lldp.c:226:11: got restricted ovs_be16
../lib/lldp/lldp.c:227:11: warning: invalid assignment: |=
../lib/lldp/lldp.c:227:11: left side has type unsigned short
../lib/lldp/lldp.c:227:11: right side has type restricted ovs_be16
../lib/lldp/lldp.c:237:15: warning: incorrect type in assignment (different
base types)
../lib/lldp/lldp.c:237:15: expected unsigned short static [unsigned]
[toplevel] [assigned] [usertype] f_uint16
../lib/lldp/lldp.c:237:15: got restricted ovs_be16
../lib/lldp/lldp.c:249:19: warning: incorrect type in assignment (different
base types)
../lib/lldp/lldp.c:249:19: expected unsigned int static [unsigned]
[toplevel] [assigned] [usertype] f_uint32
../lib/lldp/lldp.c:249:19: got restricted ovs_be32
../lib/lldp/lldp.c:255:19: warning: incorrect type in assignment (different
base types)
../lib/lldp/lldp.c:255:19: expected unsigned int static [unsigned]
[toplevel] [assigned] [usertype] f_uint32
../lib/lldp/lldp.c:255:19: got restricted ovs_be32
../lib/lldp/lldp.c:261:15: warning: invalid assignment: |=
../lib/lldp/lldp.c:261:15: left side has type unsigned short
../lib/lldp/lldp.c:261:15: right side has type restricted ovs_be16
../lib/lldp/lldp.c:269:15: warning: incorrect type in assignment (different
base types)
../lib/lldp/lldp.c:269:15: expected unsigned short static [unsigned]
[toplevel] [assigned] [usertype] f_uint16
../lib/lldp/lldp.c:269:15: got restricted ovs_be16
../lib/lldp/lldp.c:271:15: warning: invalid assignment: |=
../lib/lldp/lldp.c:271:15: left side has type unsigned short
../lib/lldp/lldp.c:271:15: right side has type restricted ovs_be16
../lib/lldp/lldp.c:299:15: warning: incorrect type in assignment (different
base types)
../lib/lldp/lldp.c:299:15: expected unsigned short static [unsigned]
[toplevel] [assigned] [usertype] f_uint16
../lib/lldp/lldp.c:299:15: got restricted ovs_be16
../lib/lldp/lldp.c:310:15: warning: invalid assignment: |=
../lib/lldp/lldp.c:310:15: left side has type unsigned short
../lib/lldp/lldp.c:310:15: right side has type restricted ovs_be16
../lib/lldp/lldp.c:320:15: warning: incorrect type in assignment (different
base types)
../lib/lldp/lldp.c:320:15: expected unsigned short static [unsigned]
[toplevel] [assigned] [usertype] f_uint16
../lib/lldp/lldp.c:320:15: got restricted ovs_be16
../lib/lldp/lldp.c:337:23: warning: incorrect type in assignment (different
base types)
../lib/lldp/lldp.c:337:23: expected unsigned short static [unsigned]
[toplevel] [assigned] [usertype] f_uint16
../lib/lldp/lldp.c:337:23: got restricted ovs_be16
../lib/lldp/lldp.c:347:15: warning: invalid assignment: |=
../lib/lldp/lldp.c:347:15: left side has type unsigned short
../lib/lldp/lldp.c:347:15: right side has type restricted ovs_be16
../lib/lldp/lldp.c:353:11: warning: incorrect type in assignment (different
base types)
../lib/lldp/lldp.c:353:11: expected unsigned short static [unsigned]
[toplevel] [assigned] [usertype] f_uint16
../lib/lldp/lldp.c:353:11: got restricted ovs_be16
../lib/lldp/lldp.c:354:11: warning: invalid assignment: |=
../lib/lldp/lldp.c:354:11: left side has type unsigned short
../lib/lldp/lldp.c:354:11: right side has type restricted ovs_be16
../lib/lldp/lldp.c:449:9: warning: incorrect type in argument 1 (different
base types)
../lib/lldp/lldp.c:449:9: expected restricted ovs_be16 [usertype] x
../lib/lldp/lldp.c:449:9: got unsigned short static [unsigned]
[toplevel] [assigned] [usertype] f_uint16
../lib/lldp/lldp.c:460:20: warning: incorrect type in argument 1 (different
base types)
../lib/lldp/lldp.c:460:20: expected restricted ovs_be16 [usertype] x
../lib/lldp/lldp.c:460:20: got unsigned short static [unsigned]
[toplevel] [assigned] [usertype] f_uint16
../lib/lldp/lldp.c:508:30: warning: incorrect type in argument 1 (different
base types)
../lib/lldp/lldp.c:508:30: expected restricted ovs_be16 [usertype] x
../lib/lldp/lldp.c:508:30: got unsigned short static [unsigned]
[toplevel] [assigned] [usertype] f_uint16
../lib/lldp/lldp.c:535:40: warning: incorrect type in argument 1 (different
base types)
../lib/lldp/lldp.c:535:40: expected restricted ovs_be16 [usertype] x
../lib/lldp/lldp.c:535:40: got unsigned short static [unsigned]
[toplevel] [assigned] [usertype] f_uint16
../lib/lldp/lldp.c:536:38: warning: incorrect type in argument 1 (different
base types)
../lib/lldp/lldp.c:536:38: expected restricted ovs_be16 [usertype] x
../lib/lldp/lldp.c:536:38: got unsigned short static [unsigned]
[toplevel] [assigned] [usertype] f_uint16
../lib/lldp/lldp.c:548:28: warning: incorrect type in argument 1 (different
base types)
../lib/lldp/lldp.c:548:28: expected restricted ovs_be32 [usertype] x
../lib/lldp/lldp.c:548:28: got unsigned int static [unsigned] [toplevel]
[assigned] [usertype] f_uint32
../lib/lldp/lldp.c:585:39: warning: incorrect type in argument 1 (different
base types)
../lib/lldp/lldp.c:585:39: expected restricted ovs_be16 [usertype] x
../lib/lldp/lldp.c:585:39: got unsigned short static [unsigned]
[toplevel] [assigned] [usertype] f_uint16
../lib/lldp/lldp.c:602:41: warning: incorrect type in argument 1 (different
base types)
../lib/lldp/lldp.c:602:41: expected restricted ovs_be16 [usertype] x
../lib/lldp/lldp.c:602:41: got unsigned short static [unsigned]
[toplevel] [assigned] [usertype] f_uint16
../lib/lldp/lldp.c:630:51: warning: incorrect type in argument 1 (different
base types)
../lib/lldp/lldp.c:630:51: expected restricted ovs_be16 [usertype] x
../lib/lldp/lldp.c:630:51: got unsigned short static [unsigned]
[toplevel] [assigned] [usertype] f_uint16
CCLD ofproto/libofproto.la
../lib/ovs-lldp.c:837:31: warning: Using plain integer as NULL pointer
ovs-lldp.c
----------
ovs-lldp.c includes one lib header using a "lib/" prefix, which is
unnecessary:
#include "lib/dynamic-string.h"
(While you're at it, it's best to put the headers in alphabetical
order, see CodingStyle.md.)
I see a lot of "static inline" function in ovs-lldp.c; see CodingStyle.md:
Functions in .c files should not normally be marked "inline", because
it does not usually help code generation and it does suppress
compilers warnings about unused functions. (Functions defined in .h
usually should be marked inline.)
chassisid_to_string() uses malloc() without checking the return value.
I suggest using xmalloc() instead. None of its callers seem to free
the output string; I think that's a memory leak.
I see a number of lines just over OVS's customary 79-character limit.
I see many uses of hash_bytes() to hash a pointer. I suggest
hash_pointer().
This file is very heavy on casts. Most of them should be removed.
Here are a few that illustrate the pattern that I would follow:
diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
index 4ef607c..0f43342 100644
--- a/lib/ovs-lldp.c
+++ b/lib/ovs-lldp.c
@@ -225,11 +225,10 @@ aa_print_lldp_and_aa_stats(struct ds *ds, struct lldp
*lldp)
return;
}
- hardware_start = ((struct lldpd_hardware *) &lldp->lldpd->g_hardware);
- hardware = (struct lldpd_hardware *) ((struct list *)
hardware_start)->next;
+ hardware_start = &lldp->lldpd->g_hardware;
+ hardware = (struct lldpd_hardware *) hardware_start->h_entries.next;
while (hardware && ( hardware != hardware_start)) {
- ds_put_format(ds, "\ttx cnt: %llu\n",
- (long long unsigned int) hardware->h_tx_cnt);
+ ds_put_format(ds, "\ttx cnt: %"PRIu64"\n", hardware->h_tx_cnt);
ds_put_format(ds, "\trx cnt: %llu\n",
(long long unsigned int) hardware->h_rx_cnt);
ds_put_format(ds, "\trx discarded cnt: %llu\n",
@@ -245,7 +244,7 @@ aa_print_lldp_and_aa_stats(struct ds *ds, struct lldp *lldp)
ds_put_format(ds, "\tdrop cnt: %llu\n",
(long long unsigned int) hardware->h_drop_cnt);
- hardware = (struct lldpd_hardware *) ((struct list *) hardware)->next;
+ hardware = (struct lldpd_hardware *) hardware->h_entries.next;
}
}
Actually, though I'm not certain what's going on (because of the whole
lldpd_hardware instance in struct lldpd, instead of just a "struct
list", which would be usual at least in OVS), it looks like the loop
in this function is just a long way to write LIST_FOR_EACH, which
makes iteration much easier to understand:
diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
index 4ef607c..437870d 100644
--- a/lib/ovs-lldp.c
+++ b/lib/ovs-lldp.c
@@ -217,7 +217,7 @@ static void
aa_print_lldp_and_aa_stats(struct ds *ds, struct lldp *lldp)
OVS_REQUIRES(mutex)
{
- struct lldpd_hardware *hardware, *hardware_start;
+ const struct lldpd_hardware *hardware;
ds_put_format(ds, "Statistics: %s\n", lldp->name);
@@ -225,11 +225,8 @@ aa_print_lldp_and_aa_stats(struct ds *ds, struct lldp
*lldp)
return;
}
- hardware_start = ((struct lldpd_hardware *) &lldp->lldpd->g_hardware);
- hardware = (struct lldpd_hardware *) ((struct list *)
hardware_start)->next;
- while (hardware && ( hardware != hardware_start)) {
- ds_put_format(ds, "\ttx cnt: %llu\n",
- (long long unsigned int) hardware->h_tx_cnt);
+ LIST_FOR_EACH (hardware, h_entries, &lldp->lldpd->g_hardware.h_entries) {
+ ds_put_format(ds, "\ttx cnt: %"PRIu64"\n", hardware->h_tx_cnt);
ds_put_format(ds, "\trx cnt: %llu\n",
(long long unsigned int) hardware->h_rx_cnt);
ds_put_format(ds, "\trx discarded cnt: %llu\n",
@@ -244,8 +241,6 @@ aa_print_lldp_and_aa_stats(struct ds *ds, struct lldp *lldp)
(long long unsigned int) hardware->h_delete_cnt);
ds_put_format(ds, "\tdrop cnt: %llu\n",
(long long unsigned int) hardware->h_drop_cnt);
-
- hardware = (struct lldpd_hardware *) ((struct list *) hardware)->next;
}
}
I see tons of these handmade list iterations. Can you switch all of
them to LIST_FOR_EACH?
In update_mapping_on_lldp(), I would remove the parentheses following
sizeof in multiple places, following CodingStyle.md:
The "sizeof" operator is unique among C operators in that it accepts
two very different kinds of operands: an expression or a type. In
general, prefer to specify an expression, e.g. "int *x =
xmalloc(sizeof *x);". When the operand of sizeof is an expression,
there is no need to parenthesize that operand, and please don't.
Here, however, I would use ARRAY_SIZE:
sizeof(lm->isid_vlan_data.isid) /
sizeof(lm->isid_vlan_data.isid[0]),
This code in update_mapping_on_lldp() looks odd. Why isn't the list
initialized whenever its containing data structure is allocated? (I
didn't track down where that happens to see if there's some good
reason.)
if (hardware->h_lport.p_isid_vlan_maps.m_entries.next == NULL &&
hardware->h_lport.p_isid_vlan_maps.m_entries.prev == NULL) {
list_init((struct list *) &hardware->h_lport.p_isid_vlan_maps);
}
The aa_get_vlan_queued() interface looks odd; why doesn't it take a
"struct list *"? It seems strange to xmalloc() just a single struct
list on the caller's behalf.
In aa_configure(), I suggest just looking at whether
s->system_description[0] is nonzero instead of calling strlen().
It looks like there are two copies of every aa_mapping_internal just
so that they can be in two hmaps. That's not necessary; you can just
add two hmap_nodes to aa_mapping_internal and then add them to
separate hmaps.
I don't think the memsets in lldp_init() are needed--aren't these
static variables, that will automatically be initialized by the
loader?
I don't see why lldp_should_process_flow() modifies wc->masks.dl_dst,
since it doesn't examine it. If anything, I'd expect it to modify
wc->masks.dl_type, since it does examine that, but that's unnecessary
since dl_type is always unwildcarded at a higher level.
In lldp_should_process_flow(), please write this:
return (ntohs(flow->dl_type) == ETH_TYPE_LLDP);
as:
return flow->dl_type == htons(ETH_TYPE_LLDP);
because the compiler can optimize it better.
Here, please put "void" on a line of its own:
void lldp_put_packet(struct lldp *lldp, struct ofpbuf *packet,
uint8_t eth_src[ETH_ADDR_LEN])
lldp_put_packet() should use xmalloc() instead of malloc(), but
actually I don't see why it's allocating memory directly at all. The
ofpbuf API should be sufficient; please use it instead of creating a
separate buffer on the side and then copying in.
I see another bare malloc() in lldp_create().
In lldp_create(), when lldpd_alloc_hardware() fails, I think I would
just call out_of_memory().
Why is there a special case for IFF_RUNNING on Windows?
In lldp_create() here, why not use xstrdup()? And the p_id is
obviously always created from a null-terminated string; why is it
copied into a length-based string?
hardware->h_lport.p_id = malloc(strlen(netdev_get_name(netdev)));
hardware->h_lport.p_id_len = strlen(netdev_get_name(netdev));
memcpy(hardware->h_lport.p_id, netdev_get_name(netdev),
strlen(netdev_get_name(netdev)));
This code here:
p = xzalloc(sizeof *p);
memcpy(p, m, sizeof *p);
can be written more simply as:
p = xmemdup(m, sizeof *p);
although again I think we can avoid having two copies.
Not sure of safety in lldp_unref(). Is it possible for a client to
get a reference to 'lldp', via 'all_llldps', between the final unref
and the hmap removal?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
