Re: [PATCH 9/9] Use timer_settime for new platforms

2014-08-28 Thread Junio C Hamano
Jonas 'Sortie' Termansen sor...@maxsi.org writes:

 setitimer() is an obsolescent XSI interface and may be removed in a
 future standard. Applications should use the core POSIX timer_settime()
 instead.

 This patch cleans up the progress reporting and changes it to try using
 timer_settime, or if that fails, setitimer. If either function is not
 provided by the system, then git-compat-util.h provides replacements
 that always fail with ENOSYS.

 It's important that code doesn't simply check if timer_settime is
 available as it can give false positives. Some systems like contemporary
 OpenBSD provides the function, but it unconditionally fails with ENOSYS
 at runtime.

 This approach allows the code using timer_settime() and setitimer() to
 be simple and readable. My first attempt used #ifdef around each use of
 timer_settime(), this quickly turned a into unmaintainable maze of
 preprocessor conditionals.

 Signed-off-by: Jonas 'Sortie' Termansen sor...@maxsi.org
 ---
  builtin/log.c | 47 ---
  progress.c| 34 +++---
  2 files changed, 67 insertions(+), 14 deletions(-)

Yuck.  I didn't look at the change very carefully, but are the two
interface so vastly different that you cannot emulate one in terms
of the other, and use a single API at the callsites, isolating the
knowledge of which kind of API is used to interact with the system
timer in one place (perhaps in compat/itimer.c)?

Having to sprinkle if (is_using_timer_settime) around means we
need to support two APIs at each and every callsite that wants timer
interrupt actions.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 9/9] Use timer_settime for new platforms

2014-08-27 Thread Jonas 'Sortie' Termansen
setitimer() is an obsolescent XSI interface and may be removed in a
future standard. Applications should use the core POSIX timer_settime()
instead.

This patch cleans up the progress reporting and changes it to try using
timer_settime, or if that fails, setitimer. If either function is not
provided by the system, then git-compat-util.h provides replacements
that always fail with ENOSYS.

It's important that code doesn't simply check if timer_settime is
available as it can give false positives. Some systems like contemporary
OpenBSD provides the function, but it unconditionally fails with ENOSYS
at runtime.

This approach allows the code using timer_settime() and setitimer() to
be simple and readable. My first attempt used #ifdef around each use of
timer_settime(), this quickly turned a into unmaintainable maze of
preprocessor conditionals.

Signed-off-by: Jonas 'Sortie' Termansen sor...@maxsi.org
---
 builtin/log.c | 47 ---
 progress.c| 34 +++---
 2 files changed, 67 insertions(+), 14 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 4389722..54c85a6 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -229,7 +229,8 @@ static void show_early_header(struct rev_info *rev, const 
char *stage, int nr)
printf(_(Final output: %d %s\n), nr, stage);
 }
 
-static struct itimerval early_output_timer;
+static int is_using_timer_settime;
+static timer_t early_output_timer;
 
 static void log_show_early(struct rev_info *revs, struct commit_list *list)
 {
@@ -271,9 +272,21 @@ static void log_show_early(struct rev_info *revs, struct 
commit_list *list)
 * trigger every second even if we're blocked on a
 * reader!
 */
-   early_output_timer.it_value.tv_sec = 0;
-   early_output_timer.it_value.tv_usec = 50;
-   setitimer(ITIMER_REAL, early_output_timer, NULL);
+   if (is_using_timer_settime) {
+   struct itimerspec value;
+   value.it_value.tv_sec = 0;
+   value.it_value.tv_nsec = 50L * 1000L;
+   value.it_interval.tv_sec = 0;
+   value.it_interval.tv_nsec = 0;
+   timer_settime(early_output_timer, 0, value, NULL);
+   } else {
+   struct itimerval value;
+   value.it_value.tv_sec = 0;
+   value.it_value.tv_usec = 50;
+   value.it_interval.tv_sec = 0;
+   value.it_interval.tv_usec = 0;
+   setitimer(ITIMER_REAL, value, NULL);
+   }
 }
 
 static void early_output(int signal)
@@ -284,6 +297,7 @@ static void early_output(int signal)
 static void setup_early_output(struct rev_info *rev)
 {
struct sigaction sa;
+   struct sigevent sev;
 
/*
 * Set up the signal handler, minimally intrusively:
@@ -305,14 +319,33 @@ static void setup_early_output(struct rev_info *rev)
 *
 * This is a one-time-only trigger.
 */
-   early_output_timer.it_value.tv_sec = 0;
-   early_output_timer.it_value.tv_usec = 10;
-   setitimer(ITIMER_REAL, early_output_timer, NULL);
+   memset(sev, 0, sizeof(sev));
+   sev.sigev_notify = SIGEV_SIGNAL;
+   sev.sigev_signo = SIGALRM;
+   if (!timer_create(CLOCK_MONOTONIC, sev, early_output_timer)) {
+   struct itimerspec value;
+   value.it_value.tv_sec = 0;
+   value.it_value.tv_nsec = 10L * 1000L;
+   value.it_interval.tv_sec = 0;
+   value.it_interval.tv_nsec = 0;
+   timer_settime(early_output_timer, 0, value, NULL);
+   is_using_timer_settime = 1;
+   } else {
+   struct itimerval value;
+   value.it_value.tv_sec = 0;
+   value.it_value.tv_usec = 10;
+   value.it_interval.tv_sec = 0;
+   value.it_interval.tv_usec = 0;
+   setitimer(ITIMER_REAL, value, NULL);
+   is_using_timer_settime = 0;
+   }
 }
 
 static void finish_early_output(struct rev_info *rev)
 {
int n = estimate_commit_count(rev, rev-commits);
+   if (is_using_timer_settime)
+   timer_delete(early_output_timer);
signal(SIGALRM, SIG_IGN);
show_early_header(rev, done, n);
 }
diff --git a/progress.c b/progress.c
index 412e6b1..b396ee1 100644
--- a/progress.c
+++ b/progress.c
@@ -38,6 +38,8 @@ struct progress {
struct throughput *throughput;
 };
 
+static int is_using_timer_settime;
+static timer_t progress_timer;
 static volatile sig_atomic_t progress_update;
 
 static void progress_interval(int signum)
@@ -48,7 +50,7 @@ static void progress_interval(int signum)
 static void set_progress_signal(void)
 {
struct sigaction sa;
-   struct itimerval v;
+   struct sigevent sev;
 
progress_update = 0;
 
@@ -58,16 +60,34 @@ static void set_progress_signal(void)
sa.sa_flags = SA_RESTART;
sigaction(SIGALRM, sa,