On 16 January 2015 at 14:50, Mario Torrecillas Rodriguez < [email protected]> wrote:
> > > On 16/01/2015 19:12, "Ola Liljedahl" <[email protected]> wrote: > > >On 16 January 2015 at 16:44, Mario Torrecillas Rodriguez > ><[email protected]> wrote: > >> Added odp_errno.c and odp_errno.h > >> Changed odp_packet_io and odp_timer to use it. > >> > >> Signed-off-by: Mario Torrecillas Rodriguez > >><[email protected]> > >> --- > >> (This code contribution is provided under the terms of agreement > >>LES-LTM-21309) > >> > >> platform/linux-generic/Makefile.am | 2 + > >> platform/linux-generic/include/api/odp_errno.h | 62 > >>++++++++++++++++++++++++++ > >> platform/linux-generic/odp_errno.c | 42 +++++++++++++++++ > >> platform/linux-generic/odp_packet_io.c | 4 +- > >> platform/linux-generic/odp_packet_socket.c | 16 +++++++ > >> platform/linux-generic/odp_timer.c | 5 ++- > >> 6 files changed, 127 insertions(+), 4 deletions(-) > >> create mode 100644 platform/linux-generic/include/api/odp_errno.h > >> create mode 100644 platform/linux-generic/odp_errno.c > >> > >> diff --git a/platform/linux-generic/Makefile.am > >>b/platform/linux-generic/Makefile.am > >> index 4535c57..3795968 100644 > >> --- a/platform/linux-generic/Makefile.am > >> +++ b/platform/linux-generic/Makefile.am > >> @@ -8,6 +8,7 @@ AM_CFLAGS += -I$(top_srcdir)/helper/include > >> include_HEADERS = \ > >> > >>$(top_srcdir)/platform/linux-generic/include/api/odp.h \ > >> > >>$(top_srcdir)/platform/linux-generic/include/api/odp_align.h \ > >> + > >>$(top_srcdir)/platform/linux-generic/include/api/odp_errno.h \ > >Should files be listed in alphabetical order? > > > >> > >>$(top_srcdir)/platform/linux-generic/include/api/odp_atomic.h \ > >> > >>$(top_srcdir)/platform/linux-generic/include/api/odp_barrier.h \ > >> > >>$(top_srcdir)/platform/linux-generic/include/api/odp_buffer.h \ > >> @@ -75,6 +76,7 @@ subdirheaders_HEADERS = \ > >> > >> __LIB__libodp_la_SOURCES = \ > >> odp_barrier.c \ > >> + odp_errno.c \ > >Order? > > > >> odp_buffer.c \ > >> odp_buffer_pool.c \ > >> odp_classification.c \ > >> diff --git a/platform/linux-generic/include/api/odp_errno.h > >>b/platform/linux-generic/include/api/odp_errno.h > >> new file mode 100644 > >> index 0000000..df8acb3 > >> --- /dev/null > >> +++ b/platform/linux-generic/include/api/odp_errno.h > >> @@ -0,0 +1,62 @@ > >> +/* Copyright (c) 2015, Linaro Limited > >> + * All rights reserved. > >> + * > >> + * SPDX-License-Identifier: BSD-3-Clause > >> + */ > >> + > >> +/** > >> + * @file > >> + * > >> + * ODP errno API > >> + */ > >> + > >> +#ifndef ODP_ERRNO_H_ > >> +#define ODP_ERRNO_H_ > >> + > >> +#ifdef __cplusplus > >> +extern "C" { > >> +#endif > >> + > >> +#include <errno.h> > >> + > >> +extern __thread int __odp_errno; > >Do we need this declaration in this public header file? Is this part > >of the public API? > Not really, only ODP implementation files should have access to it. > Perhaps I should simply create odp_errno_internal.h and have this variable > declared there (the file would not contain anything elseŠ). Then ODP > libraries would include the internal header file, and ODP applications > would include the public one. We do have odp_internal.h - maybe there ? > > >> + > >> +/** > >> +* Return latest ODP errno > >> +* > >> +* @retval 0 == no error > >> +*/ > >> +int odp_errno(void); > >> + > >> +/** > >> +* Set ODP errno to zero > >> +*/ > >> +void odp_errno_zero(void); > >Why do we need this function when we have __odp_errno public? > > > >Either use an lvalue macro or variable like __odp_errno which can be > >referenced directly. > >Or have odp_errno() and odp_errno_set(v) functions. > >Not a mix of both styles. > Applications should be able to clear the variable, but not to change it to > an arbitrary value. That¹s the reason odp_errno_zero() exists but > odp_errno_set(v) does not. > But then again, as you pointed out above, the variable should not be in > the public header file. > > > >> + > >> +/** > >> +* Print ODP errno > >> +* > >> +* Interprets the value of ODP errno as an error message, and prints it, > >> +* optionally preceding it with the custom message specified in str. > >> +* > >> +* @param str Pointer to the string to be appended > >If this custom message is optional, can you specify NULL here? > > > >> +*/ > >> +void odp_errno_print(const char *str); > >> + > >> +/** > >> +* Error message string > >> +* > >> +* Interprets the value of ODP errno, generating a string with a > >Perhaps document that it uses the system definition of errno. > > > >> +* message that describes the error. > >> +* > >> +* @param errnum Error code > >> +* > >> +* @retval Pointer to the string > >> +*/ > >> +const char *odp_errno_str(int errnum); > >> + > >> +#ifdef __cplusplus > >> +} > >> +#endif > >> + > >> +#endif > >> diff --git a/platform/linux-generic/odp_errno.c > >>b/platform/linux-generic/odp_errno.c > >> new file mode 100644 > >> index 0000000..b0ddfef > >> --- /dev/null > >> +++ b/platform/linux-generic/odp_errno.c > >> @@ -0,0 +1,42 @@ > >> +/* Copyright (c) 2015, Linaro Limited > >> + * All rights reserved. > >> + * > >> + * SPDX-License-Identifier: BSD-3-Clause > >> + */ > >> + > >> +#include <odp_errno.h> > >> +#include <string.h> > >> +#include <stdio.h> > >> + > >> +#define ODP_MAX_ERRNO_STR 200 > >> + > >> +__thread int __odp_errno; > >> + > >> +int odp_errno(void) > >> +{ > >> + return __odp_errno; > >> +} > >> + > >> +void odp_errno_zero(void) > >> +{ > >> + __odp_errno = 0; > >> +} > >> + > >> +void odp_errno_print(const char *str) > >> +{ > >> + char prefix_str[ODP_MAX_ERRNO_STR]; > >> + int n; > >> + > >> + n = snprintf(prefix_str, ODP_MAX_ERRNO_STR, "%s %s", > >> + str, strerror(__odp_errno)); > >Why pre-format the string here and risk that it does not fit into the > >buffer? Why not call printf directly? > Good point. This function was originally implemented differently (using > perror()) until I realised it would not work. I switched to printf > afterwards, and forgot to remove the pre-formatting. > > > >> + > >> + if (n > 0 && n < ODP_MAX_ERRNO_STR) > >> + printf("%s\n", prefix_str); > >> + else > >> + printf("%s\n", strerror(__odp_errno)); > >> +} > >> + > >> +const char *odp_errno_str(int errnum) > >> +{ > >> + return strerror(errnum); > >> +} > >> diff --git a/platform/linux-generic/odp_packet_io.c > >>b/platform/linux-generic/odp_packet_io.c > >> index cd109d2..c1c79d4 100644 > >> --- a/platform/linux-generic/odp_packet_io.c > >> +++ b/platform/linux-generic/odp_packet_io.c > >> @@ -18,12 +18,12 @@ > >> #include <odp_schedule_internal.h> > >> #include <odp_classification_internal.h> > >> #include <odp_debug_internal.h> > >> +#include <odp_errno.h> > >> > >> #include <string.h> > >> #include <sys/ioctl.h> > >> #include <linux/if_arp.h> > >> #include <ifaddrs.h> > >> -#include <errno.h> > >> > >> static pktio_table_t *pktio_tbl; > >> > >> @@ -259,7 +259,7 @@ odp_pktio_t odp_pktio_open(const char *dev, > >>odp_buffer_pool_t pool) > >> id = odp_pktio_lookup(dev); > >> if (id != ODP_PKTIO_INVALID) { > >> /* interface is already open */ > >> - errno = -EEXIST; > >> + __odp_errno = -EEXIST; > >> return ODP_PKTIO_INVALID; > >> } > >> > >> diff --git a/platform/linux-generic/odp_packet_socket.c > >>b/platform/linux-generic/odp_packet_socket.c > >> index da7fb2c..12aa380 100644 > >> --- a/platform/linux-generic/odp_packet_socket.c > >> +++ b/platform/linux-generic/odp_packet_socket.c > >> @@ -40,6 +40,7 @@ > >> #include <odp_align_internal.h> > >> #include <odp_debug_internal.h> > >> #include <odp_hints.h> > >> +#include <odp_errno.h> > >> > >> #include <odph_eth.h> > >> #include <odph_ip.h> > >> @@ -112,6 +113,7 @@ static int set_pkt_sock_fanout_mmap(pkt_sock_mmap_t > >>*const pkt_sock, > >> > >> err = setsockopt(sockfd, SOL_PACKET, PACKET_FANOUT, &val, > >>sizeof(val)); > >> if (err != 0) { > >> + __odp_errno = errno; > >> ODP_ERR("setsockopt(PACKET_FANOUT): %s\n", > >>strerror(errno)); > >> return -1; > >> } > >> @@ -185,6 +187,8 @@ int setup_pkt_sock(pkt_sock_t *const pkt_sock, > >>const char *netdev, > >> return sockfd; > >> > >> error: > >> + __odp_errno = errno; > >> + > >> return -1; > >> } > >> > >> @@ -195,6 +199,7 @@ error: > >> int close_pkt_sock(pkt_sock_t *const pkt_sock) > >> { > >> if (pkt_sock->sockfd != -1 && close(pkt_sock->sockfd) != 0) { > >> + __odp_errno = errno; > >> ODP_ERR("close(sockfd): %s\n", strerror(errno)); > >> return -1; > >> } > >> @@ -430,12 +435,14 @@ static int mmap_pkt_socket(void) > >> > >> int ret, sock = socket(PF_PACKET, SOCK_RAW, htons(ETH_P_ALL)); > >> if (sock == -1) { > >> + __odp_errno = errno; > >> ODP_ERR("socket(SOCK_RAW): %s\n", strerror(errno)); > >> return -1; > >> } > >> > >> ret = setsockopt(sock, SOL_PACKET, PACKET_VERSION, &ver, > >>sizeof(ver)); > >> if (ret == -1) { > >> + __odp_errno = errno; > >> ODP_ERR("setsockopt(PACKET_VERSION): %s\n", > >>strerror(errno)); > >> close(sock); > >> return -1; > >> @@ -568,6 +575,7 @@ static inline unsigned pkt_mmap_v2_tx(int sock, > >>struct ring *ring, > >> ret = sendto(sock, NULL, 0, MSG_DONTWAIT, NULL, 0); > >> if (ret == -1) { > >> if (errno != EAGAIN) { > >> + __odp_errno = errno; > >> ODP_ERR("sendto(pkt mmap): %s\n", > >>strerror(errno)); > >> return -1; > >> } > >> @@ -597,6 +605,7 @@ static int mmap_set_packet_loss_discard(int sock) > >> ret = setsockopt(sock, SOL_PACKET, PACKET_LOSS, (void > >>*)&discard, > >> sizeof(discard)); > >> if (ret == -1) { > >> + __odp_errno = errno; > >> ODP_ERR("setsockopt(PACKET_LOSS): %s\n", > >>strerror(errno)); > >> return -1; > >> } > >> @@ -623,6 +632,7 @@ static int mmap_setup_ring(int sock, struct ring > >>*ring, int type) > >> > >> ret = setsockopt(sock, SOL_PACKET, type, &ring->req, > >>sizeof(ring->req)); > >> if (ret == -1) { > >> + __odp_errno = errno; > >> ODP_ERR("setsockopt(pkt mmap): %s\n", strerror(errno)); > >> return -1; > >> } > >> @@ -630,6 +640,7 @@ static int mmap_setup_ring(int sock, struct ring > >>*ring, int type) > >> ring->rd_len = ring->rd_num * sizeof(*ring->rd); > >> ring->rd = malloc(ring->rd_len); > >> if (ring->rd == NULL) { > >> + __odp_errno = errno; > >> ODP_ERR("malloc(): %s\n", strerror(errno)); > >> return -1; > >> } > >> @@ -654,6 +665,7 @@ static int mmap_sock(pkt_sock_mmap_t *pkt_sock) > >> MAP_SHARED | MAP_LOCKED | MAP_POPULATE, sock, 0); > >> > >> if (pkt_sock->mmap_base == MAP_FAILED) { > >> + __odp_errno = errno; > >> ODP_ERR("mmap rx&tx buffer failed: %s\n", > >>strerror(errno)); > >> return -1; > >> } > >> @@ -701,6 +713,7 @@ static int mmap_bind_sock(pkt_sock_mmap_t > >>*pkt_sock, const char *netdev) > >> ret = bind(pkt_sock->sockfd, (struct sockaddr *)&pkt_sock->ll, > >> sizeof(pkt_sock->ll)); > >> if (ret == -1) { > >> + __odp_errno = errno; > >> ODP_ERR("bind(to IF): %s\n", strerror(errno)); > >> return -1; > >> } > >> @@ -719,6 +732,7 @@ static int mmap_store_hw_addr(pkt_sock_mmap_t > >>*const pkt_sock, > >> snprintf(ethreq.ifr_name, IFNAMSIZ, "%s", netdev); > >> ret = ioctl(pkt_sock->sockfd, SIOCGIFHWADDR, ðreq); > >> if (ret != 0) { > >> + __odp_errno = errno; > >> ODP_ERR("ioctl(SIOCGIFHWADDR): %s\n", strerror(errno)); > >> return -1; > >> } > >> @@ -775,6 +789,7 @@ int setup_pkt_sock_mmap(pkt_sock_mmap_t *const > >>pkt_sock, const char *netdev, > >> > >> if_idx = if_nametoindex(netdev); > >> if (if_idx == 0) { > >> + __odp_errno = errno; > >> ODP_ERR("if_nametoindex(): %s\n", strerror(errno)); > >> return -1; > >> } > >> @@ -796,6 +811,7 @@ int close_pkt_sock_mmap(pkt_sock_mmap_t *const > >>pkt_sock) > >> { > >> mmap_unmap_sock(pkt_sock); > >> if (pkt_sock->sockfd != -1 && close(pkt_sock->sockfd) != 0) { > >> + __odp_errno = errno; > >> ODP_ERR("close(sockfd): %s\n", strerror(errno)); > >> return -1; > >> } > >> diff --git a/platform/linux-generic/odp_timer.c > >>b/platform/linux-generic/odp_timer.c > >> index 3ba32a1..a8e00f8 100644 > >> --- a/platform/linux-generic/odp_timer.c > >> +++ b/platform/linux-generic/odp_timer.c > >> @@ -50,6 +50,7 @@ > >> #include <odp_time.h> > >> #include <odp_timer.h> > >> #include <odp_timer_internal.h> > >> +#include <odp_errno.h> > >> > >> #define TMO_UNUSED ((uint64_t)0xFFFFFFFFFFFFFFFF) > >> /* TMO_INACTIVE is or-ed with the expiration tick to indicate an > >>expired timer. > >> @@ -210,7 +211,7 @@ static odp_timer_pool *odp_timer_pool_new( > >> if (odp_unlikely(tp_idx >= MAX_TIMER_POOLS)) { > >> /* Restore the previous value */ > >> odp_atomic_sub_u32(&num_timer_pools, 1); > >> - errno = ENFILE; /* Table overflow */ > >> + __odp_errno = ENFILE; /* Table overflow */ > >> return NULL; > >> } > >> size_t sz0 = ODP_ALIGN_ROUNDUP(sizeof(odp_timer_pool), > >> @@ -295,7 +296,7 @@ static inline odp_timer_t > >>timer_alloc(odp_timer_pool *tp, > >> _ODP_MEMMODEL_RLS); > >> hdl = tp_idx_to_handle(tp, idx); > >> } else { > >> - errno = ENFILE; /* Reusing file table overflow */ > >> + __odp_errno = ENFILE; /* Reusing file table overflow */ > >> hdl = ODP_TIMER_INVALID; > >> } > >> odp_spinlock_unlock(&tp->lock); > >> -- > >> 1.9.1 > >> > >> > >> > >> _______________________________________________ > >> lng-odp mailing list > >> [email protected] > >> http://lists.linaro.org/mailman/listinfo/lng-odp > > > > > -- IMPORTANT NOTICE: The contents of this email and any attachments are > confidential and may also be privileged. If you are not the intended > recipient, please notify the sender immediately and do not disclose the > contents to any other person, use it for any purpose, or store or copy the > information in any medium. Thank you. > > ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, > Registered in England & Wales, Company No: 2557590 > ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, > Registered in England & Wales, Company No: 2548782 > > > _______________________________________________ > lng-odp mailing list > [email protected] > http://lists.linaro.org/mailman/listinfo/lng-odp > -- *Mike Holmes* Linaro Sr Technical Manager LNG - ODP
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
