On 09/08/2017 06:35 AM, Weglicki, MichalX wrote:
Greg,
Patch is rebased and sent to mailing list as V3 (Last patch was supposed to be
V2 - Przemek by accident sent it again as V1).
Br,
Michal.
Michal,
I'm not sure what has happened but I can't find your V3 patches in my mail but
they are in patchwork.
I tested the patches and they seem to work fine, or at least the code executes
and I didn't see any serious
regression. My ntopng/nprobe setup accepted the new template.
However, I am seeing this now:
5/Sep/2017 15:11:16 [Lua.cpp:74] ERROR: ntop_get_interface_host_info : expected
string, got nil
15/Sep/2017 15:11:16 [Lua.cpp:6658] WARNING: Script failure
[/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua][/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua:31: attempt to index global
'stats' (a nil value)]
15/Sep/2017 15:11:16 [Lua.cpp:74] ERROR: ntop_get_interface_host_info :
expected string, got nil
15/Sep/2017 15:11:15 [Lua.cpp:6658] WARNING: Script failure
[/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua][/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua:31: attempt to index global
'stats' (a nil value)]
15/Sep/2017 15:11:15 [Lua.cpp:74] ERROR: ntop_get_interface_host_info :
expected string, got nil
15/Sep/2017 15:11:12 [Lua.cpp:6658] WARNING: Script failure
[/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua][/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua:31: attempt to index global
'stats' (a nil value)]
15/Sep/2017 15:11:12 [Lua.cpp:74] ERROR: ntop_get_interface_host_info :
expected string, got nil
15/Sep/2017 15:11:09 [Lua.cpp:6658] WARNING: Script failure
[/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua][/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua:31: attempt to index global
'stats' (a nil value)]
15/Sep/2017 15:11:09 [Lua.cpp:74] ERROR: ntop_get_interface_host_info :
expected string, got nil
15/Sep/2017 15:11:09 [Lua.cpp:6658] WARNING: Script failure
[/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua][/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua:31: attempt to index global
'stats' (a nil value)]
15/Sep/2017 15:11:09 [Lua.cpp:74] ERROR: ntop_get_interface_host_info :
expected string, got nil
Also, in patch 1/2 though the interface is hard coded to Ethernet in this bit
of the patch:
+ /* Once DPDK library supports retrieving ifType we should get this value
+ * directly from DPDK rather than hardcoding it. */
+ smap_add_format(args, "if_type", "%"PRIu32, IF_TYPE_ETHERNETCSMACD);
+ smap_add_format(args, "if_descr", "%s %s", rte_version(),
+ dev_info.driver_name);
I'd like to get Darrel Ball's take on this so I've CC'd him.
In patch 2/2 I have some other comments.
Here you change the ports to point to all ports rather than tunnel ports
- struct hmap_node hmap_node; /* In struct dpif_ipfix's "tunnel_ports" hmap.
*/
+ struct hmap_node hmap_node; /* In struct dpif_ipfix's "ports" hmap. */
struct ofport *ofport; /* To retrieve port stats. */
odp_port_t odp_port;
enum dpif_ipfix_tunnel_type tunnel_type;
uint8_t tunnel_key_length;
+ uint32_t ifindex;
I didn't see any reason this can't be done but I'm worried about side effects.
Are we sure that
there are no other assumptions in the code that depend on that hmap pointing to
only tunnel ports?
I realize that patch 2/2 seems to fix that up and I didn't see any particular
reason to
doubt but I'm rather new to the code base so I thought I'd ask.
Further on I see this:
+static enum dpif_ipfix_tunnel_type
+dpif_ipfix_tunnel_type(const struct ofport *ofport) {
I think the opening curly brace should be on the next line down? I saw at
least one other instance
if this. It passes checkpatch to put the curly brace on the same line but all
other functions in
the source module put the opening curly brace of a function on the next line.
There are a couple of other checkpatch warnings and an error as well:
ERROR: Too many signoffs; are you missing Co-authored-by lines?
WARNING: Line lacks whitespace around operator
#80 FILE: ofproto/ofproto-dpif-ipfix.c:317:
ovs_be32 if_index; /* (INGRESS|EGRESS)_INTERFACE */
WARNING: Line lacks whitespace around operator
#81 FILE: ofproto/ofproto-dpif-ipfix.c:318:
ovs_be32 if_type; /* (INGRESS|EGRESS)_INTERFACE_TYPE */
Lines checked: 762, Warnings: 2, Errors: 1
The rest of patch 2/2 looks OK but I am worried about the ntop errors listed
above.
I haven't seen those before so I think they're related to this patch.
Thanks for work on this! Hopefully we can sort this all out and get your ipfix
feature enhancement applied.
Regards,
- Greg
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
