> -----Original Message-----
> From: lng-odp [mailto:[email protected]] On Behalf Of EXT 
> Stuart
> Haslam
> Sent: Friday, January 22, 2016 8:48 PM
> To: [email protected]
> 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 <[email protected]>
> ---
>  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, &eth->src, sizeof(eth->src));
> -     CU_ASSERT(ret == ODPH_ETHADDR_LEN);
> +     if (ret <= 0)
> +             memcpy(&eth->src, pktio_test_mac_src, sizeof(eth->src));
> 
>       ret = odp_pktio_mac_addr(dst, &eth->dst, sizeof(eth->dst));
> -     CU_ASSERT(ret == ODPH_ETHADDR_LEN);
> +     if (ret <= 0)
> +             memcpy(&eth->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
 */

> 
> -     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.

> -     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);
> --
> 2.1.1
> 
> _______________________________________________
> lng-odp mailing list
> [email protected]
> https://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________
lng-odp mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/lng-odp

Reply via email to