On Fri, Jan 16, 2015 at 1:12 PM, 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? > > Agreed. This seems to be an implementation model and is inconsistent with other ODP features, which take a more abstract view. > > + > > +/** > > +* 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. > I'd prefer to see accessor functions for consistency with other ODP features and not expose __odp_errno. The use of thread-local variables is an implementation choice and should not be part of the public API, especially as we now have a means of making accessor functions inlineable for performance. > > > + > > +/** > > +* 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? > Agree on allowing for NULL, but also why not send this to the ODP LOG where it can be overridden by the application like any other output? > > > +*/ > > +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? > or ODP_LOG? > > > + > > + 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 > > _______________________________________________ > lng-odp mailing list > [email protected] > http://lists.linaro.org/mailman/listinfo/lng-odp >
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
