> -----Original Message-----
> From: EXT Stuart Haslam [mailto:[email protected]]
> Sent: Thursday, January 28, 2016 1:22 PM
> To: Elo, Matias (Nokia - FI/Espoo) <[email protected]>
> Cc: [email protected]
> Subject: Re: [lng-odp] [PATCHv2 1/5] validation: pktio: make MAC address and
> promiscuous mode optional
>
> On Mon, Jan 25, 2016 at 02:56:09PM +0000, Elo, Matias (Nokia - FI/Espoo)
> wrote:
> > > -----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, ð->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.
>
True, something like the following could work.
typedef struct odp_pktio_capability_t {
/** Maximum number of input queues */
unsigned max_input_queues;
/** Maximum number of output queues */
unsigned max_output_queues;
/** Maximum number of MAC addresses */
unsigned max_mac_addr;
/** Promiscuous mode is supported */
odp_bool_t promisc_mode;
} odp_pktio_capability_t;
> --
> Stuart.
_______________________________________________
lng-odp mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/lng-odp
