Hi!
Please find my replies inline.
thanks,
Anders Widell
On 02/29/2016 12:55 PM, ramesh betham wrote:
> Hi Anders.
>
> Ack with minor comments.
>
> * Its very likely that user may pass some invalid value (say.. fd
> value) for which the current implementation is doing osaf_abort().
> Why it can't be considered error with return of "EINVAL".
>
Actually, this is a feature. Remember that the "user" here is an OpenSAF
service and never an end-user (application software). If the user of
these functions is passing an invalid argument, it is because of a bug
in the user code (i.e. a bug in OpenSAF). By calling abort(), we catch
this bug and get a core dump that makes debugging easy. If we were to
return an error code like EINVAL, chances are that the user isn't
checking the returned error code either (either not checking the error
code at all, or forgetting to check some of the possible error codes).
So then the bug might go unnoticed at this point an will only manifest
itself in some less obvious way later on when the program isn't behaving
as expected. The strategy is therefore to catch bugs early, close to the
source, and to produce a clean core dump. Besides, what is the user
supposed to do anyway when he receives an EINVAL code? The user passed
e.g. a file descriptor that he thought was valid, but in fact it is not.
We have thus detected an inconsistency and the only reasonable thing is
to restart the process to get back into a consistent state. Instead of
spreading this error handling code to every place where the function is
called, it is better to do it in one place inside the function itself.
> * Better to document that the following function won't allow
> periodic expiration notifications. Otherwise, there is a
> possibility of user setting /utmr->it_interval.tv_(n)sec/ to non-zero.
>
> /void osaf_timerfd_settime(int ufd, int flags,//
> // const struct itimerspec* utmr,//
> // struct itimerspec* otmr)//
> //{//
> // if ((flags != 0 && flags != OSAF_TFD_TIMER_ABSTIME) ||//
> // utmr->it_interval.tv_sec != 0 ||//
> // utmr->it_interval.tv_nsec != 0) {/
>
The documentation of this function mentions that utmr->it_interval must
be zero, but I can clarify it by also pointing out that this means that
periodic notifications are not supported./
/
>
> Thanks and Regards.
> Ramesh.
>
> On 12/22/2015 9:33 PM, Anders Widell wrote:
>> osaf/libs/core/common/Makefile.am | 1 +
>> osaf/libs/core/common/include/Makefile.am | 1 +
>> osaf/libs/core/common/include/osaf_timerfd.h | 131 +++++++++++++
>> osaf/libs/core/common/osaf_timerfd.c | 264
>> +++++++++++++++++++++++++++
>> 4 files changed, 397 insertions(+), 0 deletions(-)
>>
>>
>> Add the utility functions osaf_timerfd_create(), osaf_timerfd_settime(),
>> osaf_timerfd_gettime() and osaf_timerfd_close(), which have functionality
>> corresponding to the Linux functions timerfd_create(), timerfd_settime(),
>> timerfd_gettime() and close(), respectively.
>>
>> The main reason for implementing these functions here is that they are
>> missing
>> in LSB (Linux Standard Base), which means that the use these Linux functions
>> are
>> currently prohibited in OpenSAF. As an additional benefit, the variants
>> implemented here can never fail (they will abort() on failure), which means
>> that
>> the user does not have implement code for error handling.
>>
>> diff --git a/osaf/libs/core/common/Makefile.am
>> b/osaf/libs/core/common/Makefile.am
>> --- a/osaf/libs/core/common/Makefile.am
>> +++ b/osaf/libs/core/common/Makefile.am
>> @@ -34,6 +34,7 @@ libopensaf_common_la_SOURCES = \
>> osaf_utility.c \
>> osaf_poll.c \
>> osaf_time.c \
>> + osaf_timerfd.c \
>> osaf_extended_name.c \
>> nid_start_util.c \
>> saf_edu.c \
>> diff --git a/osaf/libs/core/common/include/Makefile.am
>> b/osaf/libs/core/common/include/Makefile.am
>> --- a/osaf/libs/core/common/include/Makefile.am
>> +++ b/osaf/libs/core/common/include/Makefile.am
>> @@ -34,5 +34,6 @@ noinst_HEADERS = \
>> osaf_unicode.h \
>> osaf_poll.h \
>> osaf_time.h \
>> + osaf_timerfd.h \
>> saf_error.h \
>> osaf_secutil.h
>> diff --git a/osaf/libs/core/common/include/osaf_timerfd.h
>> b/osaf/libs/core/common/include/osaf_timerfd.h
>> new file mode 100644
>> --- /dev/null
>> +++ b/osaf/libs/core/common/include/osaf_timerfd.h
>> @@ -0,0 +1,131 @@
>> +/* -*- OpenSAF -*-
>> + *
>> + * (C) Copyright 2015 The OpenSAF Foundation
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> MERCHANTABILITY
>> + * or FITNESS FOR A PARTICULAR PURPOSE. This file and program are licensed
>> + * under the GNU Lesser General Public License Version 2.1, February 1999.
>> + * The complete license can be accessed from the following location:
>> + *http://opensource.org/licenses/lgpl-license.php
>> + * See the Copying file included with the OpenSAF distribution for full
>> + * licensing terms.
>> + *
>> + * Author(s): Ericsson AB
>> + *
>> + */
>> +
>> +/** @file
>> + *
>> + * This file contains an OpenSAF replacement of the Linux functions
>> + * timerfd_create(), timerfd_settime() and timerfd_gettime(). The main
>> reason
>> + * for implementing these functions here is that they are missing in LSB
>> (Linux
>> + * Standard Base), which means that the use these Linux functions are
>> currently
>> + * prohibited in OpenSAF. As an additional benefit, the variants implemented
>> + * here can never fail (they will abort() on failure), which means that the
>> user
>> + * does not have implement code for error handling.
>> + *
>> + * When using the functions declared in this header file, you must be
>> consistent
>> + * and always use them (i.e. not mix the usage of these functions with the
>> Linux
>> + * conuterparts). Also, when using the flags parameter to
>> osaf_timerfd_create()
>> + * and osaf_timerfd_settime(), you must add the OSAF_ prefix to any flag
>> value
>> + * that you use, e.g. use OSAF_TFD_TIMER_ABSTIME instead of
>> TFD_TIMER_ABSTIME.
>> + *
>> + * Note that these functions are currently implemented using a linear search
>> + * algorithm to find the internal timer data structures. Therefore, they
>> will
>> + * not be able to handle a large number of timers efficiently. The
>> + * reccomendation is to use a maximum of around ten timers per Linux
>> process. If
>> + * more timers are needed, then these functions should be improved to use a
>> more
>> + * efficient data structure.
>> + *
>> + * The definitions in this file are for internal use within OpenSAF only.
>> + */
>> +
>> +#ifndef OPENSAF_BASE_OSAF_TIMERFD_H_
>> +#define OPENSAF_BASE_OSAF_TIMERFD_H_
>> +
>> +#include <time.h>
>> +
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif
>> +
>> +enum {
>> + OSAF_TFD_CLOEXEC = 0x80000,
>> + OSAF_TFD_NONBLOCK = 0x800
>> +};
>> +
>> +enum {
>> + OSAF_TFD_TIMER_ABSTIME = 0x1
>> +};
>> +
>> +#define OSAF_TFD_CLOEXEC OSAF_TFD_CLOEXEC
>> +#define OSAF_TFD_NONBLOCK OSAF_TFD_NONBLOCK
>> +#define OSAF_TFD_TIMER_ABSTIME OSAF_TFD_TIMER_ABSTIME
>> +
>> +/**
>> + * @brief Create a timer.
>> + *
>> + * This is a convenience function that behaves exactly like the Linux
>> function
>> + * timerfd_create(2), except that it will abort the process instead of
>> returning
>> + * an error code in case of a failure. Note that you must use the
>> + * osaf_timerfd_settime(), osaf_timerfd_gettime() and osaf_timerfd_close()
>> + * functions instead of the standard Linux functions timerfd_settime(),
>> + * timerfd_gettime() and close() for manipulating timers created using this
>> + * function.
>> + *
>> + * The @a flags parameter can be a bitwise OR of the two flags
>> OSAF_TFD_CLOEXEC
>> + * and OSAF_TFD_NONBLOCK, which have the meaning corresponding to the
>> + * TFD_CLOEXEC and TFD_NONBLOCK flags of timerfd_create(), respectively.
>> + *
>> + * The return value will always be greater than or equal to zero (i.e. this
>> + * function will never fail). To free the allocated timer, call
>> + * osaf_timerfd_close().
>> + */
>> +extern int osaf_timerfd_create(clockid_t clock_id, int flags)
>> + __attribute__ ((__nothrow__, __leaf__));
>> +
>> +/**
>> + * @brief Start, stop or change the time-out value of a timer.
>> + *
>> + * This is a convenience function that behaves exactly like the Linux
>> function
>> + * timerfd_settime(2), except that it will abort the process instead of
>> + * returning an error code in case of a failure. Aslo, it currently has the
>> + * restriction that utmr->it_interval must be zero. This function can only
>> be
>> + * used with timers created using the osaf_timerfd_create() function.
>> + *
>> + * The @a flags parameter can be either zero or have the value
>> + * OSAF_TFD_TIMER_ABSTIME, which has the meaning corresponding to the
>> + * TFD_TIMER_ABSTIME flag of the timerfd_settime() function.
>> + */
>> +extern void osaf_timerfd_settime(int ufd, int flags,
>> + const struct itimerspec* utmr,
>> + struct itimerspec* otmr)
>> + __attribute__ ((__nothrow__, __leaf__));
>> +
>> +/**
>> + * @brief Start, stop or change the time-out value of a timer.
>> + *
>> + * This is a convenience function that behaves exactly like the Linux
>> function
>> + * timerfd_gettime(2), except that it will abort the process instead of
>> + * returning an error code in case of a failure. This function can only be
>> used
>> + * with timers created using the osaf_timerfd_create() function.
>> + */
>> +extern void osaf_timerfd_gettime(int ufd, struct itimerspec* otmr)
>> + __attribute__ ((__nothrow__, __leaf__));
>> +
>> +/**
>> + * @brief Delete a timer.
>> + *
>> + * This function is a convenience function that behaves exactly like the
>> Linux
>> + * function close(2), except that it will abort the process instead of
>> returning
>> + * an error code in case of a failure. This function can only be used with
>> + * timers created using the osaf_timerfd_create() function.
>> + */
>> +extern void osaf_timerfd_close(int ufd);
>> +
>> +#ifdef __cplusplus
>> +}
>> +#endif
>> +
>> +#endif /* OPENSAF_BASE_OSAF_TIMERFD_H_ */
>> diff --git a/osaf/libs/core/common/osaf_timerfd.c
>> b/osaf/libs/core/common/osaf_timerfd.c
>> new file mode 100644
>> --- /dev/null
>> +++ b/osaf/libs/core/common/osaf_timerfd.c
>> @@ -0,0 +1,264 @@
>> +/* -*- OpenSAF -*-
>> + *
>> + * (C) Copyright 2015 The OpenSAF Foundation
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> MERCHANTABILITY
>> + * or FITNESS FOR A PARTICULAR PURPOSE. This file and program are licensed
>> + * under the GNU Lesser General Public License Version 2.1, February 1999.
>> + * The complete license can be accessed from the following location:
>> + *http://opensource.org/licenses/lgpl-license.php
>> + * See the Copying file included with the OpenSAF distribution for full
>> + * licensing terms.
>> + *
>> + * Author(s): Ericsson AB
>> + *
>> + */
>> +
>> +#ifndef _GNU_SOURCE
>> +#define _GNU_SOURCE
>> +#endif
>> +#include "osaf_timerfd.h"
>> +#include <stdlib.h>
>> +#include <errno.h>
>> +#include <pthread.h>
>> +#include <stdint.h>
>> +#include <time.h>
>> +#include <signal.h>
>> +#include <unistd.h>
>> +#include <sys/types.h>
>> +#include <sys/socket.h>
>> +#include "osaf_utility.h"
>> +#include "osaf_time.h"
>> +
>> +// Define a few constants that are missing in the LSB compiler
>> +#ifndef MSG_DONTWAIT
>> +enum {
>> + MSG_DONTWAIT = 0x40
>> +};
>> +#define MSG_DONTWAIT MSG_DONTWAIT
>> +#endif
>> +#ifndef SOCK_CLOEXEC
>> +enum {
>> + SOCK_CLOEXEC = 0x80000
>> +};
>> +#define SOCK_CLOEXEC SOCK_CLOEXEC
>> +#endif
>> +#ifndef SOCK_NONBLOCK
>> +enum {
>> + SOCK_NONBLOCK = 0x800
>> +};
>> +#define SOCK_NONBLOCK SOCK_NONBLOCK
>> +#endif
>> +#ifndef sigev_notify_function
>> +#define sigev_notify_function _sigev_un._sigev_thread._function
>> +#endif
>> +#ifndef sigev_notify_attributes
>> +#define sigev_notify_attributes _sigev_un._sigev_thread._attribute
>> +#endif
>> +
>> +/*
>> + * A data structure that keeps information about allocated timers.
>> + */
>> +struct Timer {
>> + /*
>> + * Seqence number of this timer. Used by the event handler as a key to
>> + * find the timer related to the event. If the timer has already been
>> + * deleted when the event handler is called, then the event is ignored.
>> + */
>> + int sequence_no;
>> + /*
>> + * The file descriptor that we return to the user. Part of a socket
>> + * pair together with write_socket.
>> + */
>> + int read_socket;
>> + /*
>> + * The opposite file descriptor to read_socket, which both form a
>> + * socket pair. We use this file descriptor for writing, to signal that
>> + * a timer has expired.
>> + */
>> + int write_socket;
>> + /*
>> + * The timer that we have allocated using timer_create().
>> + */
>> + timer_t timer_id;
>> + /*
>> + * Next timer in the linked list pointed to by the variable timer_list.
>> + */
>> + struct Timer* next_timer;
>> +};
>> +
>> +static struct Timer* FindTimer(int read_socket);
>> +static void EventHandler(union sigval value);
>> +
>> +/*
>> + * Mutex that protects all the shared data (i.e. all other static
>> variables in
>> + * this file).
>> + */
>> +static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
>> +
>> +/*
>> + * Pointer to the first entry in a single linked list of allocated Timer
>> + * structures. The member next_timer in the Timer structure points to the
>> next
>> + * Timer in the list.
>> + */
>> +static struct Timer* timer_list = NULL;
>> +
>> +/*
>> + * Next seqence number to be used when allocating a new timer. The sequence
>> + * number will be stored in the member sequence_no and is used by the event
>> + * handler as a key for finding the timer related to the event. This
>> variable
>> + * is initialized to a randomly chosen integer.
>> + */
>> +static int next_sequence_no = 2124226019;
>> +
>> +/*
>> + * Return a pointer to the timer whose read socket (the socket we return to
>> the
>> + * user) equals @a read_socket.
>> + */
>> +static struct Timer* FindTimer(int read_socket)
>> +{
>> + struct Timer* timer;
>> + for (timer = timer_list; timer != NULL; timer = timer->next_timer) {
>> + if (timer->read_socket == read_socket) return timer;
>> + }
>> + // Not found
>> + osaf_abort(read_socket);
>> +}
>> +
>> +/*
>> + * Event handler for the timers. This function will be called in a separate
>> + * thread when a timer has expired. The sequence number of the expired
>> timer is
>> + * found in the parameter value.sival_int, and is used as a key for looking
>> up
>> + * the timer in the single linked list timer_list. If the timer is not
>> found in
>> + * the list (since it has already been deleted), then the event will be
>> ignored.
>> + */
>> +static void EventHandler(union sigval value)
>> +{
>> + osaf_mutex_lock_ordie(&mutex);
>> + struct Timer* timer = timer_list;
>> + while (timer != NULL && timer->sequence_no != value.sival_int) {
>> + timer = timer->next_timer;
>> + }
>> + if (timer != NULL) {
>> + uint64_t expirations = 1;
>> + ssize_t result;
>> + do {
>> + result = send(timer->write_socket,
>> + &expirations, sizeof(expirations),
>> + MSG_DONTWAIT | MSG_NOSIGNAL);
>> + } while (result == -1 && errno == EINTR);
>> + if (result != sizeof(expirations)) osaf_abort(result);
>> + }
>> + osaf_mutex_unlock_ordie(&mutex);
>> +}
>> +
>> +int osaf_timerfd_create(clockid_t clock_id, int flags)
>> +{
>> + // Validate input parameters, abort on error
>> + if ((clock_id != CLOCK_REALTIME && clock_id != CLOCK_MONOTONIC) ||
>> + (flags & ~(int) (OSAF_TFD_NONBLOCK | OSAF_TFD_CLOEXEC)) != 0) {
>> + osaf_abort(flags);
>> + }
>> +
>> + int sockflags = SOCK_DGRAM;
>> + if ((flags & OSAF_TFD_NONBLOCK) == OSAF_TFD_NONBLOCK) {
>> + sockflags |= SOCK_NONBLOCK;
>> + }
>> + if ((flags & OSAF_TFD_CLOEXEC) == OSAF_TFD_CLOEXEC) {
>> + sockflags |= SOCK_CLOEXEC;
>> + }
>> +
>> + int sfd[2];
>> + if (socketpair(AF_UNIX, sockflags, 0, sfd) != 0) osaf_abort(0);
>> +
>> + struct Timer* timer = (struct Timer*) malloc(sizeof(struct Timer));
>> + if (timer == NULL) osaf_abort(0);
>> + osaf_mutex_lock_ordie(&mutex);
>> + timer->sequence_no = next_sequence_no++;
>> + osaf_mutex_unlock_ordie(&mutex);
>> + timer->read_socket = sfd[0];
>> + timer->write_socket = sfd[1];
>> +
>> + struct sigevent event;
>> + event.sigev_notify = SIGEV_THREAD;
>> + event.sigev_value.sival_int = timer->sequence_no;
>> + event.sigev_notify_function = EventHandler;
>> + event.sigev_notify_attributes = NULL;
>> + int result;
>> + for (;;) {
>> + result = timer_create(clock_id, &event, &timer->timer_id);
>> + if (result != -1 || errno != EAGAIN) break;
>> + static const struct timespec sleep_time = { 0, 10000000 };
>> + osaf_nanosleep(&sleep_time);
>> + }
>> + if (result != 0) osaf_abort(clock_id);
>> +
>> + osaf_mutex_lock_ordie(&mutex);
>> + timer->next_timer = timer_list;
>> + timer_list = timer;
>> + osaf_mutex_unlock_ordie(&mutex);
>> + return sfd[0];
>> +}
>> +
>> +void osaf_timerfd_settime(int ufd, int flags,
>> + const struct itimerspec* utmr,
>> + struct itimerspec* otmr)
>> +{
>> + if ((flags != 0 && flags != OSAF_TFD_TIMER_ABSTIME) ||
>> + utmr->it_interval.tv_sec != 0 ||
>> + utmr->it_interval.tv_nsec != 0) {
>> + osaf_abort(flags);
>> + }
>> +
>> + osaf_mutex_lock_ordie(&mutex);
>> + struct Timer* timer = FindTimer(ufd);
>> + for (;;) {
>> + uint64_t expirations;
>> + if (recv(timer->read_socket, &expirations,
>> + sizeof(expirations), MSG_DONTWAIT) < 0) {
>> + if (errno == EINTR) continue;
>> + if (errno == EAGAIN || errno == EWOULDBLOCK) break;
>> + osaf_abort(timer->read_socket);
>> + }
>> + }
>> + if (timer_settime(timer->timer_id,
>> + flags == OSAF_TFD_TIMER_ABSTIME ? TIMER_ABSTIME : 0,
>> + utmr,
>> + otmr) != 0) {
>> + osaf_abort(flags);
>> + }
>> + osaf_mutex_unlock_ordie(&mutex);
>> +}
>> +
>> +void osaf_timerfd_gettime(int ufd, struct itimerspec* otmr)
>> +{
>> + osaf_mutex_lock_ordie(&mutex);
>> + struct Timer* timer = FindTimer(ufd);
>> + if (timer_gettime(timer->timer_id, otmr) != 0) osaf_abort(ufd);
>> + osaf_mutex_unlock_ordie(&mutex);
>> +}
>> +
>> +void osaf_timerfd_close(int ufd)
>> +{
>> + osaf_mutex_lock_ordie(&mutex);
>> + struct Timer** prev = &timer_list;
>> + struct Timer* timer;
>> + for (;;) {
>> + timer = *prev;
>> + if (timer == NULL) osaf_abort(ufd);
>> + if (timer->read_socket == ufd) {
>> + *prev = timer->next_timer;
>> + break;
>> + }
>> + prev = &timer->next_timer;
>> + }
>> + osaf_mutex_unlock_ordie(&mutex);
>> +
>> + if (timer_delete(timer->timer_id) != 0 ||
>> + (close(timer->write_socket) != 0 && errno != EINTR) ||
>> + (close(timer->read_socket) != 0 && errno != EINTR)) {
>> + osaf_abort(ufd);
>> + }
>> + free(timer);
>> +}
>
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
