Re: [PATCH 9/9] Use timer_settime for new platforms
Jacob Keller writes: > diff --git a/builtin/log.c b/builtin/log.c > index 4389722b4b1e..a39e82d67eb3 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > ... > @@ -271,9 +271,12 @@ 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); > + 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); Do I see decl-after-stmt up there? -- 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
Re: [PATCH 9/9] Use timer_settime for new platforms
On Fri, 2014-08-29 at 11:02 -0700, Junio C Hamano wrote:
> Jacob Keller writes:
>
> > From: 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.
> >
> > 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.
>
> Doesn't this paragraph need tweaking? I think you lost (which is a
> good thing) "notice that timer_settime() call failed with ENOSYS and
> switch to setitimer()", no?
>
> > Clean up the progress reporting and change it to use timer_settime,
> > which will fall back to setitimer automatically if timer_settime is not
> > supported. (see git-compat-util.h for how it does this). If both
> > functions are not present, then git-compat-util.h provides replacements
> > which will always fail with ENOSYS.
>
> While this paragraph may be true if patch 8b and 9 are taken
> together, isn't what it describes mostly what 8b did, not 9?
>
> Here by 8b I mean the change to git-compat-util.h in 8; the patch
> might want to be split into two, 8a for the autoconf part whose log
> message may begin with "This function was not previously used by
> git." and 8b that adds an emulation of timer_settime() API in terms
> of setitimer() API, or the other way around.
>
> What 9 did is only "we used to use the setitmer() API to implement
> the progress reporting; now we use timer_settime() API" (yes, it is
> thanks to the abstraction given by 8, but the "callers has to only
> know about one API, not worrying about the other API" is a merit
> attributable to 8b, not this one).
>
You are correct. I can re-base these patches and split them apart a bit
better.
Regards,
Jake
> > The approach used here enables us to use a single API (timer_settime)
> > without having to worry about checking for #ifdefs or if blocks which
> > make it an unreadable nightmare. The major downside is for systems
> > without timer_settime support, they may fall back on a wrapped
> > implementation which could have subtle differences. This should be a
> > minor issue as almost all modern systems provide timer_settime support.
>
> As this paragraph.
>
> > Note that this change means that git should never use setitimer on its
> > own now, as the fallback implementation of timer_settime assumes that it
> > is the sole user of ITIMER_REAL, and timer_delete will reset the
> > ITIMER_REAL.
>
> And this one.
>
N�r��yb�X��ǧv�^�){.n�+ا���ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf
Re: [PATCH 9/9] Use timer_settime for new platforms
Jacob Keller writes: > From: 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. > > 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. Doesn't this paragraph need tweaking? I think you lost (which is a good thing) "notice that timer_settime() call failed with ENOSYS and switch to setitimer()", no? > Clean up the progress reporting and change it to use timer_settime, > which will fall back to setitimer automatically if timer_settime is not > supported. (see git-compat-util.h for how it does this). If both > functions are not present, then git-compat-util.h provides replacements > which will always fail with ENOSYS. While this paragraph may be true if patch 8b and 9 are taken together, isn't what it describes mostly what 8b did, not 9? Here by 8b I mean the change to git-compat-util.h in 8; the patch might want to be split into two, 8a for the autoconf part whose log message may begin with "This function was not previously used by git." and 8b that adds an emulation of timer_settime() API in terms of setitimer() API, or the other way around. What 9 did is only "we used to use the setitmer() API to implement the progress reporting; now we use timer_settime() API" (yes, it is thanks to the abstraction given by 8, but the "callers has to only know about one API, not worrying about the other API" is a merit attributable to 8b, not this one). > The approach used here enables us to use a single API (timer_settime) > without having to worry about checking for #ifdefs or if blocks which > make it an unreadable nightmare. The major downside is for systems > without timer_settime support, they may fall back on a wrapped > implementation which could have subtle differences. This should be a > minor issue as almost all modern systems provide timer_settime support. As this paragraph. > Note that this change means that git should never use setitimer on its > own now, as the fallback implementation of timer_settime assumes that it > is the sole user of ITIMER_REAL, and timer_delete will reset the > ITIMER_REAL. And this one. -- 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
Re: [PATCH 9/9] Use timer_settime for new platforms
On Thu, 2014-08-28 at 12:43 -0700, Junio C Hamano wrote: > Jonas 'Sortie' Termansen 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 > > --- > > 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 Agreed! The biggest difference is that timer_settime() uses nanoseconds value, which is very easy to convert microseconds into nanoseconds. In fact, you could probably easily provide a wrapper for timer_settime that works for settimer by simply converting it. This is a much better approach. (though settimer "could" lose some precision since you ahve to divide by 1000)... The other big difference is abstracting away the timer_create aspect which is ok, since we would always call timer create, and use the correct timer, but we just fall back to setitimer to use the default timer. I'll submit a patch series which does what I think should work, and will result in less if checks. Regards, Jake
Re: [PATCH 9/9] Use timer_settime for new platforms
Jonas 'Sortie' Termansen 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 > --- > 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
