| Message ID | 1453488486-12176-2-git-send-email-stuart.haslam@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Jan 25, 2016 at 02:56:09PM +0000, Elo, Matias (Nokia - FI/Espoo) wrote: > > -----Original Message----- > > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of EXT Stuart > > Haslam > > Sent: Friday, January 22, 2016 8:48 PM > > To: lng-odp@lists.linaro.org > > Subject: [lng-odp] [PATCHv2 1/5] validation: pktio: make MAC address and > > promiscuous mode optional > > > > Update the unit tests to allow for interfaces that don't have a MAC > > address and therefore also don't support promiscuous mode. > > > > The pktio promiscuous mode and MAC address APIs are intended to cope > > with the common case that an interface has a single default MAC > > address. The default MAC is the one to be used as the source address > > when originating packets from that interface, it is also what's used > > to filter packets when promiscuous mode is disabled. However in some > > cases this doesn't apply, a switch port or one end of an IPC pipe may > > not have a MAC address for example. > > > > The test will also now restore the state to how it was at the beginning > > of the test, which it didn't necessarily do previously. > > > > Signed-off-by: Stuart Haslam <stuart.haslam@linaro.org> > > --- > > test/validation/pktio/pktio.c | 80 +++++++++++++++++++++++++++++++-------- > > ---- > > 1 file changed, 58 insertions(+), 22 deletions(-) > > > > diff --git a/test/validation/pktio/pktio.c b/test/validation/pktio/pktio.c > > index 5923969..55e5471 100644 > > --- a/test/validation/pktio/pktio.c > > +++ b/test/validation/pktio/pktio.c > > @@ -79,6 +79,11 @@ pkt_segmented_e pool_segmentation = > > PKT_POOL_UNSEGMENTED; > > > > odp_pool_t pool[MAX_NUM_IFACES] = {ODP_POOL_INVALID, > > ODP_POOL_INVALID}; > > > > +/* the default source and destination MAC addresses to be used if the > > + * test interface doesn't return a MAC address via odp_pktio_mac_addr() */ > > +static const char pktio_test_mac_src[] = {0x02, 0x03, 0x04, 0x05, 0x06, 0x07}; > > +static const char pktio_test_mac_dst[] = {0x02, 0x03, 0x04, 0x05, 0x06, 0x08}; > > + > > static void set_pool_len(odp_pool_param_t *params) > > { > > switch (pool_segmentation) { > > @@ -103,10 +108,12 @@ static void pktio_pkt_set_macs(odp_packet_t pkt, > > int ret; > > > > ret = odp_pktio_mac_addr(src, ð->src, sizeof(eth->src)); > > - CU_ASSERT(ret == ODPH_ETHADDR_LEN); > > + if (ret <= 0) > > + memcpy(ð->src, pktio_test_mac_src, sizeof(eth->src)); > > > > ret = odp_pktio_mac_addr(dst, ð->dst, sizeof(eth->dst)); > > - CU_ASSERT(ret == ODPH_ETHADDR_LEN); > > + if (ret <= 0) > > + memcpy(ð->dst, pktio_test_mac_dst, sizeof(eth->dst)); > > } > > > > static uint32_t pktio_pkt_set_seq(odp_packet_t pkt) > > @@ -610,32 +617,67 @@ void pktio_test_mtu(void) > > void pktio_test_promisc(void) > > { > > int ret; > > + int enabled; > > > > odp_pktio_t pktio = create_pktio(0, ODP_PKTIN_MODE_SCHED, > > ODP_PKTOUT_MODE_SEND); > > CU_ASSERT_FATAL(pktio != ODP_PKTIO_INVALID); > > > > - ret = odp_pktio_promisc_mode_set(pktio, 1); > > - CU_ASSERT(0 == ret); > > + /* read the current promisuous state */ > > + enabled = odp_pktio_promisc_mode(pktio); > > + CU_ASSERT(enabled == 1 || enabled == 0 || enabled < 0); > > > > - /* Verify that promisc mode set */ > > - ret = odp_pktio_promisc_mode(pktio); > > - CU_ASSERT(1 == ret); > > + /* attempt to set to the same state */ > > + ret = odp_pktio_promisc_mode_set(pktio, enabled); > > + if (ret < 0) { > > + /* failed, that's ok, some interfaces don't support it, but > > + * they must set odp_errno.. */ > > + CU_ASSERT(0 != odp_errno()); > > + } else { > > + /* attempt to change to the other state */ > > + ret = odp_pktio_promisc_mode_set(pktio, !enabled); > > The packet_io API doesn't specify that errno must be set, so the value cannot be checked here. > I would suggest adding a new 'not supported' return value to odp_pktio_promisc_mode_set() > and odp_pktio_promisc_mode() functions. > > For example: > /** > * Enable/Disable promiscuous mode on a packet IO interface. > * > * @param[in] pktio Packet IO handle. > * @param[in] enable 1 to enable, 0 to disable. > * > * @retval 0 on success > * @retval -1 on not supported > * @retval <-1 on failure > */ > I prefer errno for this purpose, and it's in line with the API guidelines - http://docs.opendataplane.org/master/linux-generic-doxygen-html/api_guide_lines.html#errno But you're right the errno check here isn't in line with the API documentation as it's promoting the SHOULD in the guidelines to a MUST. > > > > - ret = odp_pktio_promisc_mode_set(pktio, 0); > > - CU_ASSERT(0 == ret); > > + /* changed the state successfully, read back and verify */ > > + ret = odp_pktio_promisc_mode(pktio); > > + CU_ASSERT(!enabled == ret); > > > > - /* Verify that promisc mode is not set */ > > - ret = odp_pktio_promisc_mode(pktio); > > - CU_ASSERT(0 == ret); > > + /* change it back to original state */ > > + ret = odp_pktio_promisc_mode_set(pktio, enabled); > > + CU_ASSERT(0 == ret); > > + > > + ret = odp_pktio_promisc_mode(pktio); > > + CU_ASSERT(enabled == ret); > > + } > > > > ret = odp_pktio_close(pktio); > > CU_ASSERT(ret == 0); > > } > > > > +static void pktio_print_mac(unsigned char *mac_addr, int mac_len) > > +{ > > + int len = 0; > > + int i; > > + char mac_str[(ODP_PKTIO_MACADDR_MAXSIZE * 3) + 1]; > > + > > + if (mac_len <= 0) { > > + printf("none ... "); > > + return; > > + } > > + > > + for (i = 0; i < mac_len; ++i) { > > + len += snprintf(mac_str + len, sizeof(mac_str) - len, > > + "%02hhx:", mac_addr[i]); > > + } > > + > > + if (len) { > > + mac_str[len - 1] = '\0'; > > + printf("%s ... ", mac_str); > > + } > > +} > > + > > void pktio_test_mac(void) > > { > > - unsigned char mac_addr[ODPH_ETHADDR_LEN]; > > + unsigned char mac_addr[ODP_PKTIO_MACADDR_MAXSIZE]; > > int mac_len; > > int ret; > > odp_pktio_t pktio; > > @@ -644,18 +686,12 @@ void pktio_test_mac(void) > > ODP_PKTOUT_MODE_SEND); > > CU_ASSERT_FATAL(pktio != ODP_PKTIO_INVALID); > > > > - printf("testing mac for %s\n", iface_name[0]); > > + printf(" MAC for %s is ", iface_name[0]); > > > > mac_len = odp_pktio_mac_addr(pktio, mac_addr, sizeof(mac_addr)); > > The current packet_io API basically assumes that the underlying layer is Ethernet --> odp_pktio_mac_addr(), > odp_pktio_promisc_mode_set(), and odp_pktio_promisc_mode_get() are always valid calls. Would it be useful > to define a new API call for fetching the underlying type (e.g. Ethernet, VALE, PCAP)? This way the application > could avoid calling unsupported API functions. > These features aren't specific to Ethernet, and they're not necessarily available even if you are on Ethernet (e.g. an Ethernet switch port may not have it's own MAC address, as is the case with VALE). I suppose it would be a good idea to add flags to the odp_pktio_capability_t structure to allow an application to check ahead of time. One flag for Ethernet (which would imply requiring MAC addressing) and another for whether promiscuous mode is supported is probably enough for now.
diff --git a/test/validation/pktio/pktio.c b/test/validation/pktio/pktio.c index 5923969..55e5471 100644 --- a/test/validation/pktio/pktio.c +++ b/test/validation/pktio/pktio.c @@ -79,6 +79,11 @@ pkt_segmented_e pool_segmentation = PKT_POOL_UNSEGMENTED; odp_pool_t pool[MAX_NUM_IFACES] = {ODP_POOL_INVALID, ODP_POOL_INVALID}; +/* the default source and destination MAC addresses to be used if the + * test interface doesn't return a MAC address via odp_pktio_mac_addr() */ +static const char pktio_test_mac_src[] = {0x02, 0x03, 0x04, 0x05, 0x06, 0x07}; +static const char pktio_test_mac_dst[] = {0x02, 0x03, 0x04, 0x05, 0x06, 0x08}; + static void set_pool_len(odp_pool_param_t *params) { switch (pool_segmentation) { @@ -103,10 +108,12 @@ static void pktio_pkt_set_macs(odp_packet_t pkt, int ret; ret = odp_pktio_mac_addr(src, ð->src, sizeof(eth->src)); - CU_ASSERT(ret == ODPH_ETHADDR_LEN); + if (ret <= 0) + memcpy(ð->src, pktio_test_mac_src, sizeof(eth->src)); ret = odp_pktio_mac_addr(dst, ð->dst, sizeof(eth->dst)); - CU_ASSERT(ret == ODPH_ETHADDR_LEN); + if (ret <= 0) + memcpy(ð->dst, pktio_test_mac_dst, sizeof(eth->dst)); } static uint32_t pktio_pkt_set_seq(odp_packet_t pkt) @@ -610,32 +617,67 @@ void pktio_test_mtu(void) void pktio_test_promisc(void) { int ret; + int enabled; odp_pktio_t pktio = create_pktio(0, ODP_PKTIN_MODE_SCHED, ODP_PKTOUT_MODE_SEND); CU_ASSERT_FATAL(pktio != ODP_PKTIO_INVALID); - ret = odp_pktio_promisc_mode_set(pktio, 1); - CU_ASSERT(0 == ret); + /* read the current promisuous state */ + enabled = odp_pktio_promisc_mode(pktio); + CU_ASSERT(enabled == 1 || enabled == 0 || enabled < 0); - /* Verify that promisc mode set */ - ret = odp_pktio_promisc_mode(pktio); - CU_ASSERT(1 == ret); + /* attempt to set to the same state */ + ret = odp_pktio_promisc_mode_set(pktio, enabled); + if (ret < 0) { + /* failed, that's ok, some interfaces don't support it, but + * they must set odp_errno.. */ + CU_ASSERT(0 != odp_errno()); + } else { + /* attempt to change to the other state */ + ret = odp_pktio_promisc_mode_set(pktio, !enabled); - ret = odp_pktio_promisc_mode_set(pktio, 0); - CU_ASSERT(0 == ret); + /* changed the state successfully, read back and verify */ + ret = odp_pktio_promisc_mode(pktio); + CU_ASSERT(!enabled == ret); - /* Verify that promisc mode is not set */ - ret = odp_pktio_promisc_mode(pktio); - CU_ASSERT(0 == ret); + /* change it back to original state */ + ret = odp_pktio_promisc_mode_set(pktio, enabled); + CU_ASSERT(0 == ret); + + ret = odp_pktio_promisc_mode(pktio); + CU_ASSERT(enabled == ret); + } ret = odp_pktio_close(pktio); CU_ASSERT(ret == 0); } +static void pktio_print_mac(unsigned char *mac_addr, int mac_len) +{ + int len = 0; + int i; + char mac_str[(ODP_PKTIO_MACADDR_MAXSIZE * 3) + 1]; + + if (mac_len <= 0) { + printf("none ... "); + return; + } + + for (i = 0; i < mac_len; ++i) { + len += snprintf(mac_str + len, sizeof(mac_str) - len, + "%02hhx:", mac_addr[i]); + } + + if (len) { + mac_str[len - 1] = '\0'; + printf("%s ... ", mac_str); + } +} + void pktio_test_mac(void) { - unsigned char mac_addr[ODPH_ETHADDR_LEN]; + unsigned char mac_addr[ODP_PKTIO_MACADDR_MAXSIZE]; int mac_len; int ret; odp_pktio_t pktio; @@ -644,18 +686,12 @@ void pktio_test_mac(void) ODP_PKTOUT_MODE_SEND); CU_ASSERT_FATAL(pktio != ODP_PKTIO_INVALID); - printf("testing mac for %s\n", iface_name[0]); + printf(" MAC for %s is ", iface_name[0]); mac_len = odp_pktio_mac_addr(pktio, mac_addr, sizeof(mac_addr)); - CU_ASSERT(ODPH_ETHADDR_LEN == mac_len); - - printf(" %X:%X:%X:%X:%X:%X ", - mac_addr[0], mac_addr[1], mac_addr[2], - mac_addr[3], mac_addr[4], mac_addr[5]); + CU_ASSERT(0 == mac_len || ODP_PKTIO_MACADDR_MAXSIZE >= mac_len); - /* Fail case: wrong addr_size. Expected <0. */ - mac_len = odp_pktio_mac_addr(pktio, mac_addr, 2); - CU_ASSERT(mac_len < 0); + pktio_print_mac(mac_addr, mac_len); ret = odp_pktio_close(pktio); CU_ASSERT(0 == ret);
Update the unit tests to allow for interfaces that don't have a MAC address and therefore also don't support promiscuous mode. The pktio promiscuous mode and MAC address APIs are intended to cope with the common case that an interface has a single default MAC address. The default MAC is the one to be used as the source address when originating packets from that interface, it is also what's used to filter packets when promiscuous mode is disabled. However in some cases this doesn't apply, a switch port or one end of an IPC pipe may not have a MAC address for example. The test will also now restore the state to how it was at the beginning of the test, which it didn't necessarily do previously. Signed-off-by: Stuart Haslam <stuart.haslam@linaro.org> --- test/validation/pktio/pktio.c | 80 +++++++++++++++++++++++++++++++------------ 1 file changed, 58 insertions(+), 22 deletions(-)