[PATCHv5,02/18] api: odp_pktio.h: odp_pktio_mac_addr() return chars written or error

Message ID 1422982108-13813-3-git-send-email-ola.liljedahl@linaro.org
State New
Headers show

Commit Message

Ola Liljedahl Feb. 3, 2015, 4:48 p.m.
Added define ODP_PKTIO_MACADDRSIZE which specifies the recommended
output buffer size for odp_pktio_mac_addr().
odp_pktio_mac_addr() takes output buffer size as input and returns
number
of chars written (on success), <0 on failure.
Updated the implementation.
Updated all usages in example and test programs.

Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org>
---
(This document/code contribution attached is provided under the terms of
agreement LES-LTM-21309)

 include/odp/api/packet_io.h                          | 20 +++++++++++++-------
 .../linux-generic/include/odp/plat/packet_io_types.h |  2 ++
 platform/linux-generic/odp_packet_io.c               | 11 ++++++-----
 test/validation/odp_pktio.c                          |  8 ++++----
 4 files changed, 25 insertions(+), 16 deletions(-)

Comments

Ola Liljedahl Feb. 4, 2015, 9:43 a.m. | #1
On 4 February 2015 at 10:07, Savolainen, Petri (NSN - FI/Espoo)
<petri.savolainen@nsn.com> wrote:
>
>
>> -----Original Message-----
>> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
>> bounces@lists.linaro.org] On Behalf Of ext Ola Liljedahl
>> Sent: Tuesday, February 03, 2015 6:48 PM
>> To: lng-odp@lists.linaro.org
>> Subject: [lng-odp] [PATCHv5 02/18] api: odp_pktio.h: odp_pktio_mac_addr()
>> return chars written or error
>>
>> Added define ODP_PKTIO_MACADDRSIZE which specifies the recommended
>> output buffer size for odp_pktio_mac_addr().
>> odp_pktio_mac_addr() takes output buffer size as input and returns
>> number
>> of chars written (on success), <0 on failure.
>> Updated the implementation.
>> Updated all usages in example and test programs.
>>
>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org>
>> ---
>> (This document/code contribution attached is provided under the terms of
>> agreement LES-LTM-21309)
>>
>>  include/odp/api/packet_io.h                          | 20 +++++++++++++--
>> -----
>>  .../linux-generic/include/odp/plat/packet_io_types.h |  2 ++
>>  platform/linux-generic/odp_packet_io.c               | 11 ++++++-----
>>  test/validation/odp_pktio.c                          |  8 ++++----
>>  4 files changed, 25 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h
>> index da7bc3f..0c8af0c 100644
>> --- a/include/odp/api/packet_io.h
>> +++ b/include/odp/api/packet_io.h
>> @@ -18,6 +18,7 @@
>>  extern "C" {
>>  #endif
>>
>> +#include <sys/types.h>
>>
>>  /** @defgroup odp_packet_io ODP PACKET IO
>>   *  Operations on a packet.
>> @@ -39,6 +40,11 @@ extern "C" {
>>   * odp_pktio_t value to indicate any port
>>   */
>>
>> +/*
>> + * @def ODP_PKTIO_MACADDRSIZE
>> + * Minimum size of output buffer for odp_pktio_mac_addr()
>
> Should this be defined as ODP_PKTIO_MACADDRSIZE_MAX or similar.

> If the interface support two sizes this is the larger, although the configured and currently used size would be the smaller.
This is just a buffer size suitable for all forseeable MAC addresses.
It is not related to the size of any MAC address currently used by
some interface (except it must be large enough).

>
> I want to avoid applications mixing this into protocol definitions like this
I agree that we want to avoid confusion like you show an example of
below. So a better name would be appreciated, I can use
ODP_PKTIO_MACADDRSIZE_MAX as you propose above.

>
> struct ether_hdr {
>   uint8_t dst_mac[ODP_PKTIO_MACADDRSIZE];
>   uint8_t src_mac[ODP_PKTIO_MACADDRSIZE];
> ...
> };
>
>
>> + */
>> +
>>  /**
>>   * Open an ODP packet IO instance
>>   *
>> @@ -167,16 +173,16 @@ int odp_pktio_promisc_mode_set(odp_pktio_t id,
>> odp_bool_t enable);
>>  int odp_pktio_promisc_mode(odp_pktio_t id);
>>
>>  /**
>> - * Get the default MAC address of a packet IO interface.
>> + * Get the native MAC address of a packet IO interface.
>
> Why this is now defined as "native". Does it mean the address burned into the device. What if SW has overwritten that? Is it still native?
Is it still the "default" MAC address if SW has overwritten it? It's
not the same default anymore.

> Default is referring to the default address, whatever that maybe.
The use of "default" does not make any sense to me (it not like the
interface will insert this default MAC address into all packet sent
from that interface). Now I think we should just drop the word
altogether. "Get the MAC address of a packet IO interface". Skip any
over specification of semantics we can't even agree on.

>
>>   *
>> - * @param    id        ODP packet IO handle.
>> - * @param[out]       mac_addr  Storage for MAC address of the packet IO
>> interface.
>> - * @param    addr_size Storage size for the address
>> + * @param    hdl  ODP packet IO handle
>
> This parameter renaming is not needed. All other packet_io.h API calls are using "id" here. Parameter naming should be constant throughout the API. Do not rename it in this patch.
Yes I made parameter naming constant through the API, I changed all
instances of "id" to "hdl" (short for handle seems we seem to like
abbreviated parameter names).

>
> I agree that "id" is not the best name for that parameter, but that's how it is in this API right now. We need another patch for renaming that over all the API calls.
It is a piss poor name when the explanation a few character away then
properly explains it as a "packet IO handle" (and not "packet IO
identifier").

>
> Also "hdl" is not a good parameter name for an object. The name should link to the object type, like "pktio" (for odp_pktio_t type) in this case. Parameter list shows up in Doxygen function documentation without types.
Yes "pktio" is even better. I will change it.

If we had had an architecture document to start with, we could have
had naming conventions for function parameters etc and avoided all of
these discussions in the last minute. I just want the documentation to
be clear and consistent and we are far away from that. We really need
someone to go through all the header files and make them consistent. I
was focusing on return values (because these are important from a
semantic perspective) but got distracted when I saw some of these
glaring inconsistencies.


>
> -Petri
>
>
>> + * @param[out]       mac_addr  Output buffer (use ODP_PKTIO_MACADDRSIZE)
>> + * @param       size Size of output buffer
>>   *
>> - * @retval Number of bytes written on success, 0 on failure.
>> + * @return Number of bytes written to buffer
>> + * @retval <0 on failure
>>   */
>> -size_t odp_pktio_mac_addr(odp_pktio_t id, void *mac_addr,
>> -                       size_t addr_size);
>> +ssize_t odp_pktio_mac_addr(odp_pktio_t hdl, void *mac_addr, ssize_t
>> size);
>>
>>  /**
>>   * Setup per-port default class-of-service.
>> diff --git a/platform/linux-generic/include/odp/plat/packet_io_types.h
>> b/platform/linux-generic/include/odp/plat/packet_io_types.h
>> index a6bbacf..61dca13 100644
>> --- a/platform/linux-generic/include/odp/plat/packet_io_types.h
>> +++ b/platform/linux-generic/include/odp/plat/packet_io_types.h
>> @@ -23,6 +23,8 @@ extern "C" {
>>   *  @{
>>   */
>>
>> +#define ODP_PKTIO_MACADDRSIZE 16
>> +
>>  typedef uint32_t odp_pktio_t;
>>
>>  #define ODP_PKTIO_INVALID 0
>> diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-
>> generic/odp_packet_io.c
>> index bdb690e..f156dd3 100644
>> --- a/platform/linux-generic/odp_packet_io.c
>> +++ b/platform/linux-generic/odp_packet_io.c
>> @@ -798,18 +798,19 @@ int odp_pktio_promisc_mode(odp_pktio_t id)
>>  }
>>
>>
>> -size_t odp_pktio_mac_addr(odp_pktio_t id, void *mac_addr,
>> -                    size_t addr_size)
>> +ssize_t odp_pktio_mac_addr(odp_pktio_t id, void *mac_addr, ssize_t
>> addr_size)
>>  {
>>       pktio_entry_t *entry;
>>
>> -     if (addr_size < ETH_ALEN)
>> -             return 0;
>> +     if (addr_size < ETH_ALEN) {
>> +             /* Output buffer too small */
>> +             return -1;
>> +     }
>>
>>       entry = get_pktio_entry(id);
>>       if (entry == NULL) {
>>               ODP_DBG("pktio entry %d does not exist\n", id);
>> -             return 0;
>> +             return -1;
>>       }
>>
>>       lock_entry(entry);
>> diff --git a/test/validation/odp_pktio.c b/test/validation/odp_pktio.c
>> index 84121f5..e1ca793 100644
>> --- a/test/validation/odp_pktio.c
>> +++ b/test/validation/odp_pktio.c
>> @@ -450,22 +450,22 @@ static void test_odp_pktio_promisc(void)
>>  static void test_odp_pktio_mac(void)
>>  {
>>       unsigned char mac_addr[ODPH_ETHADDR_LEN];
>> -     size_t mac_len;
>> +     ssize_t mac_len;
>>       int ret;
>>       odp_pktio_t pktio = create_pktio(iface_name[0]);
>>
>>       printf("testing mac for %s\n", iface_name[0]);
>>
>> -     mac_len = odp_pktio_mac_addr(pktio, mac_addr, ODPH_ETHADDR_LEN);
>> +     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]);
>>
>> -     /* Fail case: wrong addr_size. Expected 0. */
>> +     /* Fail case: wrong addr_size. Expected <0. */
>>       mac_len = odp_pktio_mac_addr(pktio, mac_addr, 2);
>> -     CU_ASSERT(0 == mac_len);
>> +     CU_ASSERT(mac_len < 0);
>>
>>       ret = odp_pktio_close(pktio);
>>       CU_ASSERT(0 == ret);
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp

Patch

diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h
index da7bc3f..0c8af0c 100644
--- a/include/odp/api/packet_io.h
+++ b/include/odp/api/packet_io.h
@@ -18,6 +18,7 @@ 
 extern "C" {
 #endif
 
+#include <sys/types.h>
 
 /** @defgroup odp_packet_io ODP PACKET IO
  *  Operations on a packet.
@@ -39,6 +40,11 @@  extern "C" {
  * odp_pktio_t value to indicate any port
  */
 
+/*
+ * @def ODP_PKTIO_MACADDRSIZE
+ * Minimum size of output buffer for odp_pktio_mac_addr()
+ */
+
 /**
  * Open an ODP packet IO instance
  *
@@ -167,16 +173,16 @@  int odp_pktio_promisc_mode_set(odp_pktio_t id, odp_bool_t enable);
 int odp_pktio_promisc_mode(odp_pktio_t id);
 
 /**
- * Get the default MAC address of a packet IO interface.
+ * Get the native MAC address of a packet IO interface.
  *
- * @param	id	  ODP packet IO handle.
- * @param[out]	mac_addr  Storage for MAC address of the packet IO interface.
- * @param	addr_size Storage size for the address
+ * @param	hdl  ODP packet IO handle
+ * @param[out]	mac_addr  Output buffer (use ODP_PKTIO_MACADDRSIZE)
+ * @param       size Size of output buffer
  *
- * @retval Number of bytes written on success, 0 on failure.
+ * @return Number of bytes written to buffer
+ * @retval <0 on failure
  */
-size_t odp_pktio_mac_addr(odp_pktio_t id, void *mac_addr,
-			  size_t addr_size);
+ssize_t odp_pktio_mac_addr(odp_pktio_t hdl, void *mac_addr, ssize_t size);
 
 /**
  * Setup per-port default class-of-service.
diff --git a/platform/linux-generic/include/odp/plat/packet_io_types.h b/platform/linux-generic/include/odp/plat/packet_io_types.h
index a6bbacf..61dca13 100644
--- a/platform/linux-generic/include/odp/plat/packet_io_types.h
+++ b/platform/linux-generic/include/odp/plat/packet_io_types.h
@@ -23,6 +23,8 @@  extern "C" {
  *  @{
  */
 
+#define ODP_PKTIO_MACADDRSIZE 16
+
 typedef uint32_t odp_pktio_t;
 
 #define ODP_PKTIO_INVALID 0
diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c
index bdb690e..f156dd3 100644
--- a/platform/linux-generic/odp_packet_io.c
+++ b/platform/linux-generic/odp_packet_io.c
@@ -798,18 +798,19 @@  int odp_pktio_promisc_mode(odp_pktio_t id)
 }
 
 
-size_t odp_pktio_mac_addr(odp_pktio_t id, void *mac_addr,
-		       size_t addr_size)
+ssize_t odp_pktio_mac_addr(odp_pktio_t id, void *mac_addr, ssize_t addr_size)
 {
 	pktio_entry_t *entry;
 
-	if (addr_size < ETH_ALEN)
-		return 0;
+	if (addr_size < ETH_ALEN) {
+		/* Output buffer too small */
+		return -1;
+	}
 
 	entry = get_pktio_entry(id);
 	if (entry == NULL) {
 		ODP_DBG("pktio entry %d does not exist\n", id);
-		return 0;
+		return -1;
 	}
 
 	lock_entry(entry);
diff --git a/test/validation/odp_pktio.c b/test/validation/odp_pktio.c
index 84121f5..e1ca793 100644
--- a/test/validation/odp_pktio.c
+++ b/test/validation/odp_pktio.c
@@ -450,22 +450,22 @@  static void test_odp_pktio_promisc(void)
 static void test_odp_pktio_mac(void)
 {
 	unsigned char mac_addr[ODPH_ETHADDR_LEN];
-	size_t mac_len;
+	ssize_t mac_len;
 	int ret;
 	odp_pktio_t pktio = create_pktio(iface_name[0]);
 
 	printf("testing mac for %s\n", iface_name[0]);
 
-	mac_len = odp_pktio_mac_addr(pktio, mac_addr, ODPH_ETHADDR_LEN);
+	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]);
 
-	/* Fail case: wrong addr_size. Expected 0. */
+	/* Fail case: wrong addr_size. Expected <0. */
 	mac_len = odp_pktio_mac_addr(pktio, mac_addr, 2);
-	CU_ASSERT(0 == mac_len);
+	CU_ASSERT(mac_len < 0);
 
 	ret = odp_pktio_close(pktio);
 	CU_ASSERT(0 == ret);