| Message ID | 1422982108-13813-3-git-send-email-ola.liljedahl@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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);
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(-)