{"thread":{"id":"31343","subject":"[PATCH 1/2] Support for setitimer() on platforms lacking it","startedAt":"2012-08-24T10:39:54Z","lastAt":"2012-09-05T09:59:37Z","messageCount":20,"participants":["Joachim Schmitz","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"197764","messageId":"003301cd81e4$cd68daa0$683a8fe0$@schmitz-digital.de","threadId":"31343","inReplyTo":null,"subject":"[PATCH 1/2] Support for setitimer() on platforms lacking it","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-08-24T10:39:54Z","receivedAt":"2012-08-24T10:39:54Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"\nImplementation includes getitimer(), but for now it is static.\nSupports ITIMER_REAL only.\n\nSigned-off-by: Joachim Schmitz <jojo@schmitz-digital.de>\n---\nMay need a header file for ITIMER_*, struct itimerval and the prototypes,\nBut for now, and the HP NonStop platform this isn't needed, here\n<sys/time> has ITIMER_* and struct timeval, and the prototypes can \nvo into git-compat-util.h for now (Patch 2/2) \n\n compat/itimer.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 50 insertions(+)\n create mode 100644 compat/itimer.c\n\ndiff --git a/compat/itimer.c b/compat/itimer.c\nnew file mode 100644\nindex 0000000..713f1ff\n--- /dev/null\n+++ b/compat/itimer.c\n@@ -0,0 +1,50 @@\n+#include \"../git-compat-util.h\"\n+\n+static int git_getitimer(int which, struct itimerval *value)\n+{\n+\tint ret = 0;\n+\n+\tswitch (which) {\n+\t\tcase ITIMER_REAL:\n+\t\t\tvalue->it_value.tv_usec = 0;\n+\t\t\tvalue->it_value.tv_sec = alarm(0);\n+\t\t\tret = 0; /* if alarm() fails, we get a SIGLIMIT */\n+\t\t\tbreak;\n+\t\tcase ITIMER_VIRTUAL: /* FALLTHRU */\n+\t\tcase ITIMER_PROF: errno = ENOTSUP; ret = -1; break;\n+\t\tdefault: errno = EINVAL; ret = -1;\n+\t}\n+\treturn ret;\n+}\n+\n+int git_setitimer(int which, const struct itimerval *value,\n+\t\t\t\tstruct itimerval *ovalue)\n+{\n+\tint ret = 0;\n+\n+\tif (!value\n+\t\t|| value->it_value.tv_usec < 0\n+\t\t|| value->it_value.tv_usec > 1000000\n+\t\t|| value->it_value.tv_sec < 0) {\n+\t\terrno = EINVAL;\n+\t\treturn -1;\n+\t}\n+\n+\telse if (ovalue)\n+\t\tif (!git_getitimer(which, ovalue))\n+\t\t\treturn -1; /* errno set in git_getitimer() */\n+\n+\telse\n+\tswitch (which) {\n+\t\tcase ITIMER_REAL:\n+\t\t\talarm(value->it_value.tv_sec +\n+\t\t\t\t(value->it_value.tv_usec > 0) ? 1 : 0);\n+\t\t\tret = 0; /* if alarm() fails, we get a SIGLIMIT */\n+\t\t\tbreak;\n+\t\tcase ITIMER_VIRTUAL: /* FALLTHRU */\n+\t\tcase ITIMER_PROF: errno = ENOTSUP; ret = -1; break;\n+\t\tdefault: errno = EINVAL; ret = -1;\n+\t}\n+\n+\treturn ret;\n+}\n-- \n1.7.12\n"},{"id":"198012","messageId":"7vr4qqzsbe.fsf@alter.siamese.dyndns.org","threadId":"31343","inReplyTo":"003301cd81e4$cd68daa0$683a8fe0$@schmitz-digital.de","subject":"Re: [PATCH 1/2] Support for setitimer() on platforms lacking it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-28T20:15:33Z","receivedAt":"2012-08-28T20:15:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Joachim Schmitz\" <jojo@schmitz-digital.de> writes:\n\n> Implementation includes getitimer(), but for now it is static.\n> Supports ITIMER_REAL only.\n>\n> Signed-off-by: Joachim Schmitz <jojo@schmitz-digital.de>\n> ---\n> May need a header file for ITIMER_*, struct itimerval and the prototypes,\n> But for now, and the HP NonStop platform this isn't needed, here\n> <sys/time> has ITIMER_* and struct timeval, and the prototypes can \n> vo into git-compat-util.h for now (Patch 2/2) \n>\n>  compat/itimer.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++\n>  1 file changed, 50 insertions(+)\n>  create mode 100644 compat/itimer.c\n>\n> diff --git a/compat/itimer.c b/compat/itimer.c\n> new file mode 100644\n> index 0000000..713f1ff\n> --- /dev/null\n> +++ b/compat/itimer.c\n> @@ -0,0 +1,50 @@\n> +#include \"../git-compat-util.h\"\n> +\n> +static int git_getitimer(int which, struct itimerval *value)\n> +{\n> +\tint ret = 0;\n> +\n> +\tswitch (which) {\n> +\t\tcase ITIMER_REAL:\n> +\t\t\tvalue->it_value.tv_usec = 0;\n> +\t\t\tvalue->it_value.tv_sec = alarm(0);\n> +\t\t\tret = 0; /* if alarm() fails, we get a SIGLIMIT */\n> +\t\t\tbreak;\n> +\t\tcase ITIMER_VIRTUAL: /* FALLTHRU */\n> +\t\tcase ITIMER_PROF: errno = ENOTSUP; ret = -1; break;\n> +\t\tdefault: errno = EINVAL; ret = -1;\n> +\t}\n\nJust a style thing, but we align case arms and switch statements,\nlike this:\n\n\tswitch (which) {\n        case ...:\n        \tstmt;\n                break;\n\tdefault:\n        \tstmt;\n                break;\n\t}\n\t\nBecause alarm() runs in integral seconds granularity, this could\nreturn 0.0 sec (i.e. \"do not re-trigger this alarm any more\") in\novalue after setting alarm(1) (via git_setitimer()) and calling this\nfunction (via git_setitimer() again) before the timer expires, no?\nIs it a desired behaviour?\n\nWhat I am most worried about is that callers _might_ take this\nemulation too seriously, grab the remainder from getitimer(), and\ndrives a future call to getitimer() with the returned value, and\naccidentally cause the \"recurring\" nature of the request to be\ndisabled.\n\nI see no existing code calls setitimer() with non-NULL ovalue, and I\ndo not think we would add a new caller that would do so in any time\nsoon, so it may not be a bad idea to drop support of returning the\nremaining timer altogether from this emulation layer (just like\ngiving anything other than ITIMER_REAL gives us ENOTSUP).  That\nwould sidestep the whole \"we cannot answer how many milliseconds are\nstill remaining on the timer when using emulation based on alarm()\".\n\n> +int git_setitimer(int which, const struct itimerval *value,\n> +\t\t\t\tstruct itimerval *ovalue)\n> +{\n> +\tint ret = 0;\n> +\n> +\tif (!value\n> +\t\t|| value->it_value.tv_usec < 0\n> +\t\t|| value->it_value.tv_usec > 1000000\n> +\t\t|| value->it_value.tv_sec < 0) {\n> +\t\terrno = EINVAL;\n> +\t\treturn -1;\n> +\t}\n> +\n> +\telse if (ovalue)\n> +\t\tif (!git_getitimer(which, ovalue))\n> +\t\t\treturn -1; /* errno set in git_getitimer() */\n> +\n> +\telse\n> +\tswitch (which) {\n> +\t\tcase ITIMER_REAL:\n> +\t\t\talarm(value->it_value.tv_sec +\n> +\t\t\t\t(value->it_value.tv_usec > 0) ? 1 : 0);\n\nWhy is this capped to 1 second?  Is this because no existing code\nuses the timer for anything other than 1 second or shorter?  If that\nis the case, that needs at least some documenting (or a possibly\nsupport for longer expiration, if it is not too cumbersome to add).\n\n> +\t\t\tret = 0; /* if alarm() fails, we get a SIGLIMIT */\n> +\t\t\tbreak;\n> +\t\tcase ITIMER_VIRTUAL: /* FALLTHRU */\n> +\t\tcase ITIMER_PROF: errno = ENOTSUP; ret = -1; break;\n\nPlease don't add a misleading \"fallthru\" label here.  We do not say\n\"fallthru\" when \"two case arms do _exactly_ the same thing\".  Only\nwhen the one arm does some pre-action before the common action, i.e.\n\n\tswitch (which) {\n        case one:\n        \tdo some thing specific to one;\n                /* fallthru */\n\tcase two:\n\t\tdo some thing common between one and two;\n\t\tbreak;\n\t}                \n        \nwe label it \"fallthru\" to make it clear to the readers that it is\nnot \"missing a break\" but is deliberate.\n\n> +\t\tdefault: errno = EINVAL; ret = -1;\n> +\t}\n> +\n> +\treturn ret;\n> +}\n\nThanks.\n"},{"id":"198108","messageId":"002201cd86ce$285841b0$7908c510$@schmitz-digital.de","threadId":"31343","inReplyTo":"7vr4qqzsbe.fsf@alter.siamese.dyndns.org","subject":"RE: [PATCH 1/2] Support for setitimer() on platforms lacking it","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-08-30T16:40:25Z","receivedAt":"2012-08-30T16:40:25Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"> From: Junio C Hamano [mailto:gitster@pobox.com]\n> Sent: Tuesday, August 28, 2012 10:16 PM\n> To: Joachim Schmitz\n> Cc: git@vger.kernel.org\n> Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it\n> \n> \"Joachim Schmitz\" <jojo@schmitz-digital.de> writes:\n> \n> > Implementation includes getitimer(), but for now it is static.\n> > Supports ITIMER_REAL only.\n> >\n> > Signed-off-by: Joachim Schmitz <jojo@schmitz-digital.de>\n> > ---\n> > May need a header file for ITIMER_*, struct itimerval and the prototypes,\n> > But for now, and the HP NonStop platform this isn't needed, here\n> > <sys/time> has ITIMER_* and struct timeval, and the prototypes can\n> > vo into git-compat-util.h for now (Patch 2/2)\n> >\n> >  compat/itimer.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++\n> >  1 file changed, 50 insertions(+)\n> >  create mode 100644 compat/itimer.c\n> >\n> > diff --git a/compat/itimer.c b/compat/itimer.c\n> > new file mode 100644\n> > index 0000000..713f1ff\n> > --- /dev/null\n> > +++ b/compat/itimer.c\n> > @@ -0,0 +1,50 @@\n> > +#include \"../git-compat-util.h\"\n> > +\n> > +static int git_getitimer(int which, struct itimerval *value)\n> > +{\n> > +\tint ret = 0;\n> > +\n> > +\tswitch (which) {\n> > +\t\tcase ITIMER_REAL:\n> > +\t\t\tvalue->it_value.tv_usec = 0;\n> > +\t\t\tvalue->it_value.tv_sec = alarm(0);\n> > +\t\t\tret = 0; /* if alarm() fails, we get a SIGLIMIT */\n> > +\t\t\tbreak;\n> > +\t\tcase ITIMER_VIRTUAL: /* FALLTHRU */\n> > +\t\tcase ITIMER_PROF: errno = ENOTSUP; ret = -1; break;\n> > +\t\tdefault: errno = EINVAL; ret = -1;\n> > +\t}\n> \n> Just a style thing, but we align case arms and switch statements,\n> like this:\n> \n> \tswitch (which) {\n>         case ...:\n>         \tstmt;\n>                 break;\n> \tdefault:\n>         \tstmt;\n>                 break;\n> \t}\n\nOK, I'll fix the syle\n\n> Because alarm() runs in integral seconds granularity, this could\n> return 0.0 sec (i.e. \"do not re-trigger this alarm any more\") in\n> ovalue after setting alarm(1) (via git_setitimer()) and calling this\n> function (via git_setitimer() again) before the timer expires, no?\n> Is it a desired behaviour?\n\nUnintentional, never really thought about this.\n \n> What I am most worried about is that callers _might_ take this\n> emulation too seriously, grab the remainder from getitimer(), and\n> drives a future call to getitimer() with the returned value, and\n> accidentally cause the \"recurring\" nature of the request to be\n> disabled.\n> \n> I see no existing code calls setitimer() with non-NULL ovalue, and I\n> do not think we would add a new caller that would do so in any time\n> soon, so it may not be a bad idea to drop support of returning the\n> remaining timer altogether from this emulation layer (just like\n> giving anything other than ITIMER_REAL gives us ENOTSUP).  That\n> would sidestep the whole \"we cannot answer how many milliseconds are\n> still remaining on the timer when using emulation based on alarm()\".\n\nShould we leave tv_usec untouched then? That was we round up on the next (and subsequent?) round(s). Or just set to ENOTSUP in\nsetitimer if ovalue is !NULL?\n\n> > +int git_setitimer(int which, const struct itimerval *value,\n> > +\t\t\t\tstruct itimerval *ovalue)\n> > +{\n> > +\tint ret = 0;\n> > +\n> > +\tif (!value\n> > +\t\t|| value->it_value.tv_usec < 0\n> > +\t\t|| value->it_value.tv_usec > 1000000\n> > +\t\t|| value->it_value.tv_sec < 0) {\n> > +\t\terrno = EINVAL;\n> > +\t\treturn -1;\n> > +\t}\n> > +\n> > +\telse if (ovalue)\n> > +\t\tif (!git_getitimer(which, ovalue))\n> > +\t\t\treturn -1; /* errno set in git_getitimer() */\n> > +\n> > +\telse\n> > +\tswitch (which) {\n> > +\t\tcase ITIMER_REAL:\n> > +\t\t\talarm(value->it_value.tv_sec +\n> > +\t\t\t\t(value->it_value.tv_usec > 0) ? 1 : 0);\n> \n> Why is this capped to 1 second?  Is this because no existing code\n> uses the timer for anything other than 1 second or shorter?  If that\n> is the case, that needs at least some documenting (or a possibly\n> support for longer expiration, if it is not too cumbersome to add).\n\nAs you mention alarm() has only seconds resolution. It is tv_sec plus 1 if there are tv_usecs > 0, it is rounding up, so we don't\ncancel the alarm() if tv_sec is 0 but tv_usec is not. Looks OK to me?\n \n> > +\t\t\tret = 0; /* if alarm() fails, we get a SIGLIMIT */\n> > +\t\t\tbreak;\n> > +\t\tcase ITIMER_VIRTUAL: /* FALLTHRU */\n> > +\t\tcase ITIMER_PROF: errno = ENOTSUP; ret = -1; break;\n> \n> Please don't add a misleading \"fallthru\" label here.  We do not say\n> \"fallthru\" when \"two case arms do _exactly_ the same thing\".  Only\n> when the one arm does some pre-action before the common action, i.e.\n> \n> \tswitch (which) {\n>         case one:\n>         \tdo some thing specific to one;\n>                 /* fallthru */\n> \tcase two:\n> \t\tdo some thing common between one and two;\n> \t\tbreak;\n> \t}\n> \n> we label it \"fallthru\" to make it clear to the readers that it is\n> not \"missing a break\" but is deliberate.\n\nI'll fix those too.\n\n> > +\t\tdefault: errno = EINVAL; ret = -1;\n> > +\t}\n> > +\n> > +\treturn ret;\n> > +}\n> \n> Thanks.\n\nBye, Jojo\n"},{"id":"198110","messageId":"7vfw74s3oy.fsf@alter.siamese.dyndns.org","threadId":"31343","inReplyTo":"002201cd86ce$285841b0$7908c510$@schmitz-digital.de","subject":"Re: [PATCH 1/2] Support for setitimer() on platforms lacking it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-30T17:13:49Z","receivedAt":"2012-08-30T17:13:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Joachim Schmitz\" <jojo@schmitz-digital.de> writes:\n\n>> I see no existing code calls setitimer() with non-NULL ovalue, and I\n>> do not think we would add a new caller that would do so in any time\n>> soon, so it may not be a bad idea to drop support of returning the\n>> remaining timer altogether from this emulation layer (just like\n>> giving anything other than ITIMER_REAL gives us ENOTSUP).  That\n>> would sidestep the whole \"we cannot answer how many milliseconds are\n>> still remaining on the timer when using emulation based on alarm()\".\n>\n> Should we leave tv_usec untouched then? That was we round up on\n> the next (and subsequent?) round(s). Or just set to ENOTSUP in\n> setitimer if ovalue is !NULL?\n\nI was alluding to the latter.\n\n>> > +\tswitch (which) {\n>> > +\t\tcase ITIMER_REAL:\n>> > +\t\t\talarm(value->it_value.tv_sec +\n>> > +\t\t\t\t(value->it_value.tv_usec > 0) ? 1 : 0);\n>> \n>> Why is this capped to 1 second?  Is this because no existing code\n>> uses the timer for anything other than 1 second or shorter?  If that\n>> is the case, that needs at least some documenting (or a possibly\n>> support for longer expiration, if it is not too cumbersome to add).\n>\n> As you mention alarm() has only seconds resolution. It is tv_sec\n> plus 1 if there are tv_usecs > 0, it is rounding up, so we don't\n> cancel the alarm() if tv_sec is 0 but tv_usec is not. Looks OK to\n> me?\n\nCan a caller use setitimer to be notified in 5 seconds?\n"},{"id":"198111","messageId":"003101cd86d4$14b494a0$3e1dbde0$@schmitz-digital.de","threadId":"31343","inReplyTo":"7vfw74s3oy.fsf@alter.siamese.dyndns.org","subject":"RE: [PATCH 1/2] Support for setitimer() on platforms lacking it","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-08-30T17:22:48Z","receivedAt":"2012-08-30T17:22:48Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"> From: Junio C Hamano [mailto:gitster@pobox.com]\n> Sent: Thursday, August 30, 2012 7:14 PM\n> To: Joachim Schmitz\n> Cc: git@vger.kernel.org\n> Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it\n> \n> \"Joachim Schmitz\" <jojo@schmitz-digital.de> writes:\n> \n> >> I see no existing code calls setitimer() with non-NULL ovalue, and I\n> >> do not think we would add a new caller that would do so in any time\n> >> soon, so it may not be a bad idea to drop support of returning the\n> >> remaining timer altogether from this emulation layer (just like\n> >> giving anything other than ITIMER_REAL gives us ENOTSUP).  That\n> >> would sidestep the whole \"we cannot answer how many milliseconds are\n> >> still remaining on the timer when using emulation based on alarm()\".\n> >\n> > Should we leave tv_usec untouched then? That was we round up on\n> > the next (and subsequent?) round(s). Or just set to ENOTSUP in\n> > setitimer if ovalue is !NULL?\n> \n> I was alluding to the latter.\n\nOK, will do that then.\n\n> >> > +\tswitch (which) {\n> >> > +\t\tcase ITIMER_REAL:\n> >> > +\t\t\talarm(value->it_value.tv_sec +\n> >> > +\t\t\t\t(value->it_value.tv_usec > 0) ? 1 : 0);\n> >>\n> >> Why is this capped to 1 second?  Is this because no existing code\n> >> uses the timer for anything other than 1 second or shorter?  If that\n> >> is the case, that needs at least some documenting (or a possibly\n> >> support for longer expiration, if it is not too cumbersome to add).\n> >\n> > As you mention alarm() has only seconds resolution. It is tv_sec\n> > plus 1 if there are tv_usecs > 0, it is rounding up, so we don't\n> > cancel the alarm() if tv_sec is 0 but tv_usec is not. Looks OK to\n> > me?\n> \n> Can a caller use setitimer to be notified in 5 seconds?\n\nYes, by setting tv_sec to 5 and tv_usec to 0, or be setting tv_sec to 4 and tv_usec to something > 0.\n\nUnless I screwed up the operator precedence?\nTo make it clearer (any possibly correct?):\n\n\tswitch (which) {\n\t\tcase ITIMER_REAL:\n\t\t\talarm(value->it_value.tv_sec +\n\t\t\t\t((value->it_value.tv_usec > 0) ? 1 : 0));\n\nOr even just\n\tswitch (which) {\n\t\tcase ITIMER_REAL:\n\t\t\talarm(value->it_value.tv_sec + (value->it_value.tv_usec > 0));\n"},{"id":"198182","messageId":"003d01cd8827$34e90180$9ebb0480$@schmitz-digital.de","threadId":"31343","inReplyTo":"7vfw74s3oy.fsf@alter.siamese.dyndns.org","subject":"RE: [PATCH 1/2] Support for setitimer() on platforms lacking it","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-09-01T09:50:22Z","receivedAt":"2012-09-01T09:50:22Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"> From: Joachim Schmitz [mailto:jojo@schmitz-digital.de]\n> Sent: Thursday, August 30, 2012 7:23 PM\n> To: 'Junio C Hamano'\n> Cc: 'git@vger.kernel.org'\n> Subject: RE: [PATCH 1/2] Support for setitimer() on platforms lacking it\n> \n> > From: Junio C Hamano [mailto:gitster@pobox.com]\n> > Sent: Thursday, August 30, 2012 7:14 PM\n> > To: Joachim Schmitz\n> > Cc: git@vger.kernel.org\n> > Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it\n> >\n> > \"Joachim Schmitz\" <jojo@schmitz-digital.de> writes:\n> >\n> > >> I see no existing code calls setitimer() with non-NULL ovalue, and I\n> > >> do not think we would add a new caller that would do so in any time\n> > >> soon, so it may not be a bad idea to drop support of returning the\n> > >> remaining timer altogether from this emulation layer (just like\n> > >> giving anything other than ITIMER_REAL gives us ENOTSUP).  That\n> > >> would sidestep the whole \"we cannot answer how many milliseconds are\n> > >> still remaining on the timer when using emulation based on alarm()\".\n> > >\n> > > Should we leave tv_usec untouched then? That was we round up on\n> > > the next (and subsequent?) round(s). Or just set to ENOTSUP in\n> > > setitimer if ovalue is !NULL?\n> >\n> > I was alluding to the latter.\n> \n> OK, will do that then.\n> \n> > >> > +\tswitch (which) {\n> > >> > +\t\tcase ITIMER_REAL:\n> > >> > +\t\t\talarm(value->it_value.tv_sec +\n> > >> > +\t\t\t\t(value->it_value.tv_usec > 0) ? 1 : 0);\n> > >>\n> > >> Why is this capped to 1 second?  Is this because no existing code\n> > >> uses the timer for anything other than 1 second or shorter?  If that\n> > >> is the case, that needs at least some documenting (or a possibly\n> > >> support for longer expiration, if it is not too cumbersome to add).\n> > >\n> > > As you mention alarm() has only seconds resolution. It is tv_sec\n> > > plus 1 if there are tv_usecs > 0, it is rounding up, so we don't\n> > > cancel the alarm() if tv_sec is 0 but tv_usec is not. Looks OK to\n> > > me?\n> >\n> > Can a caller use setitimer to be notified in 5 seconds?\n> \n> Yes, by setting tv_sec to 5 and tv_usec to 0, or be setting tv_sec to 4 and tv_usec to something > 0.\n> \n> Unless I screwed up the operator precedence?\n> To make it clearer (any possibly correct?):\n> \n> \tswitch (which) {\n> \t\tcase ITIMER_REAL:\n> \t\t\talarm(value->it_value.tv_sec +\n> \t\t\t\t((value->it_value.tv_usec > 0) ? 1 : 0));\n> \n> Or even just\n> \tswitch (which) {\n> \t\tcase ITIMER_REAL:\n> \t\t\talarm(value->it_value.tv_sec + (value->it_value.tv_usec > 0));\n\nOK, here it goes again, not yet as a patch, just plain code for comment:\n\n$ cat itimer.c\n/* \n * Rely on system headers (<sys/time.h>) to contain struct itimerval\n * and git-compat-util.h to have the prototype for git_getitimer().\n * As soon as there's a platform where that is not the case, we'd need\n * an itimer .h.\n */\n#include \"../git-compat-util.h\"\n\n#ifndef NO_GETITIMER /* not yet needed anywhere else in git */\nstatic\n#endif\nint git_getitimer(int which, struct itimerval *value)\n{\n\tint ret = 0;\n\n\tif (!value) {\n\t\terrno = EFAULT;\n\t\treturn -1;\n\t}\n\n\tswitch (which) {\n\tcase ITIMER_REAL:\n#if 0\n\t\tvalue->it_value.tv_usec = 0;\n\t\tvalue->it_value.tv_sec = alarm(0);\n\t\tret = 0; /* if alarm() fails, we get a SIGLIMIT */\n\t\tbreak;\n#else\n\t\t/*\n\t\t * As an emulation via alarm(0) won't tell us how many\n\t\t * usecs are left, we don't support it altogether.\n\t\t */\n#endif\n\tcase ITIMER_VIRTUAL:\n\tcase ITIMER_PROF:\n\t\terrno = ENOTSUP;\n\t\tret = -1;\n\t\tbreak;\n\tdefault:\n\t\terrno = EINVAL;\n\t\tret = -1;\n\t\tbreak;\n\t}\n\treturn ret;\n}\n\nint git_setitimer(int which, const struct itimerval *value,\n\t\t\t\tstruct itimerval *ovalue)\n{\n\tint ret = 0;\n\n\tif (!value ) {\n\t\terrno = EFAULT;\n\t\treturn -1;\n\t}\n\n\tif ( value->it_value.tv_sec < 0\n\t    || value->it_value.tv_usec > 1000000\n\t    || value->it_value.tv_usec < 0) {\n\t\terrno = EINVAL;\n\t\treturn -1;\n\t}\n\n\tif ((ovalue) && (git_getitimer(which, ovalue) == -1))\n\t\treturn -1; /* errno set in git_getitimer() */\n\n\tswitch (which) {\n\tcase ITIMER_REAL:\n\t\t /* If tv_usec is > 0, round up to next full sec */\n\t\talarm(value->it_value.tv_sec + (value->it_value.tv_usec > 0));\n\t\tret = 0; /* if alarm() fails, we get a SIGLIMIT */\n\t\tbreak;\n\tcase ITIMER_VIRTUAL:\n\t\tcase ITIMER_PROF:\n\t\terrno = ENOTSUP;\n\t\tret = -1;\n\t\tbreak;\n\tdefault:\n\t\terrno = EINVAL;\n\t\tret = -1;\n\t\tbreak;\n\t}\n\n\treturn ret;\n}\n\nWould this pass muster? The previous version had a bug too, of ovalue was !NULL the switch was never reached.\n\nBye, Jojo\n"},{"id":"198229","messageId":"7vpq64f935.fsf@alter.siamese.dyndns.org","threadId":"31343","inReplyTo":"003d01cd8827$34e90180$9ebb0480$@schmitz-digital.de","subject":"Re: [PATCH 1/2] Support for setitimer() on platforms lacking it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-02T20:43:43Z","receivedAt":"2012-09-02T20:43:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Joachim Schmitz\" <jojo@schmitz-digital.de> writes:\n\n>> > > Should we leave tv_usec untouched then? That was we round up on\n>> > > the next (and subsequent?) round(s). Or just set to ENOTSUP in\n>> > > setitimer if ovalue is !NULL?\n>> >\n>> > I was alluding to the latter.\n>> \n>> OK, will do that then.\n\nThanks.\n\n>> Unless I screwed up the operator precedence?\n\nI think you did, but not in the version we see below.\n\n> int git_setitimer(int which, const struct itimerval *value,\n> \t\t\t\tstruct itimerval *ovalue)\n> {\n> \tint ret = 0;\n>\n> \tif (!value ) {\n\nStyle: space before ')'?\n\n> \t\terrno = EFAULT;\n> \t\treturn -1;\n\nEFAULT is good ;-)\n\nThe emulation in mingw.c 6072fc3 (Windows: Implement setitimer() and\nsigaction()., 2007-11-13) may want to be tightened in a similar way.\n\n> \t}\n>\n> \tif ( value->it_value.tv_sec < 0\n\nStyle: space after ')'?\n\n> \t    || value->it_value.tv_usec > 1000000\n> \t    || value->it_value.tv_usec < 0) {\n> \t\terrno = EINVAL;\n> \t\treturn -1;\n> \t}\n>\n> \tif ((ovalue) && (git_getitimer(which, ovalue) == -1))\n> \t\treturn -1; /* errno set in git_getitimer() */\n\nAs nobody passes non-NULL ovalue to setitimer(), I think we should\ninstead get rid of git_getitmier() implemenation, and change this to\n\n\tif (ovalue) {\n        \terrno = ENOTSUP;\n                return -1;\n\t}\n\nwhich is how I understood what \"the latter\" in the paragraph I\nquoted from you above meant.\n\n> \tswitch (which) {\n> \tcase ITIMER_REAL:\n> \t\t /* If tv_usec is > 0, round up to next full sec */\n> \t\talarm(value->it_value.tv_sec + (value->it_value.tv_usec > 0));\n\nOK.\n\n> \t\tret = 0; /* if alarm() fails, we get a SIGLIMIT */\n> \t\tbreak;\n> \tcase ITIMER_VIRTUAL:\n> \t\tcase ITIMER_PROF:\n> \t\terrno = ENOTSUP;\n> \t\tret = -1;\n> \t\tbreak;\n> \tdefault:\n> \t\terrno = EINVAL;\n> \t\tret = -1;\n> \t\tbreak;\n> \t}\n>\n> \treturn ret;\n> }\n\nOther than that, looks good.\n\nThanks.\n"},{"id":"198241","messageId":"000d01cd89b6$d5ba6c30$812f4490$@schmitz-digital.de","threadId":"31343","inReplyTo":"7vpq64f935.fsf@alter.siamese.dyndns.org","subject":"RE: [PATCH 1/2] Support for setitimer() on platforms lacking it","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-09-03T09:31:01Z","receivedAt":"2012-09-03T09:31:01Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"> From: Junio C Hamano [mailto:gitster@pobox.com]\n> Sent: Sunday, September 02, 2012 10:44 PM\n> To: Joachim Schmitz\n> Cc: git@vger.kernel.org; Johannes Sixt\n> Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it\n> \n> \"Joachim Schmitz\" <jojo@schmitz-digital.de> writes:\n> \n> >> > > Should we leave tv_usec untouched then? That was we round up on\n> >> > > the next (and subsequent?) round(s). Or just set to ENOTSUP in\n> >> > > setitimer if ovalue is !NULL?\n> >> >\n> >> > I was alluding to the latter.\n> >>\n> >> OK, will do that then.\n> \n> Thanks.\n> \n> >> Unless I screwed up the operator precedence?\n> \n> I think you did, but not in the version we see below.\n> \n> > int git_setitimer(int which, const struct itimerval *value,\n> > \t\t\t\tstruct itimerval *ovalue)\n> > {\n> > \tint ret = 0;\n> >\n> > \tif (!value ) {\n> \n> Style: space before ')'?\n\nWill fix.\n \n> > \t\terrno = EFAULT;\n> > \t\treturn -1;\n> \n> EFAULT is good ;-)\n\nThat's what 'man setitimer()' on Linux says to happen if invalid value is found.\n \n> The emulation in mingw.c 6072fc3 (Windows: Implement setitimer() and\n> sigaction()., 2007-11-13) may want to be tightened in a similar way.\n\n\nHmm, I see that there the errors are handled differently, like this:\n\n        if (ovalue != NULL)\n                return errno = EINVAL,\n                        error(\"setitimer param 3 != NULL not implemented\");\n\nShould this be done in my setitimer() too? Or rather be left to the caller?\nI tend to the later.\n\n> > \t}\n> >\n> > \tif ( value->it_value.tv_sec < 0\n> \n> Style: space after ')'?\n\nAfter '(', I guess? Will fix.\n \n> > \t    || value->it_value.tv_usec > 1000000\n> > \t    || value->it_value.tv_usec < 0) {\n> > \t\terrno = EINVAL;\n> > \t\treturn -1;\n> > \t}\n> >\n> > \tif ((ovalue) && (git_getitimer(which, ovalue) == -1))\n> > \t\treturn -1; /* errno set in git_getitimer() */\n> \n> As nobody passes non-NULL ovalue to setitimer(), I think we should\n> instead get rid of git_getitmier() implemenation, and change this to\n\nTrue.\n \n> \tif (ovalue) {\n>         \terrno = ENOTSUP;\n>                 return -1;\n> \t}\n>\n> which is how I understood what \"the latter\" in the paragraph I\n> quoted from you above meant.\n\nOK, will do this and then I'll rename the entire file into getitimer.c.\n \n> > \tswitch (which) {\n> > \tcase ITIMER_REAL:\n> > \t\t /* If tv_usec is > 0, round up to next full sec */\n> > \t\talarm(value->it_value.tv_sec + (value->it_value.tv_usec > 0));\n> \n> OK.\n> \n> > \t\tret = 0; /* if alarm() fails, we get a SIGLIMIT */\n> > \t\tbreak;\n> > \tcase ITIMER_VIRTUAL:\n> > \t\tcase ITIMER_PROF:\n> > \t\terrno = ENOTSUP;\n> > \t\tret = -1;\n> > \t\tbreak;\n> > \tdefault:\n> > \t\terrno = EINVAL;\n> > \t\tret = -1;\n> > \t\tbreak;\n> > \t}\n> >\n> > \treturn ret;\n> > }\n> \n> Other than that, looks good.\n> \n> Thanks.\n\nI had a closer look at the places in git where setitimer() is used. It is in 2 files, progress.c and builtin/log.c.\nIn progress.c :\nstatic void set_progress_signal(void)\n{\n        struct sigaction sa;\n        struct itimerval v;\n\n        progress_update = 0;\n\n        memset(&sa, 0, sizeof(sa));\n        sa.sa_handler = progress_interval;\n        sigemptyset(&sa.sa_mask);\n        sa.sa_flags = SA_RESTART;\n        sigaction(SIGALRM, &sa, NULL);\n\n        v.it_interval.tv_sec = 1;\n        v.it_interval.tv_usec = 0;\n        v.it_value = v.it_interval;\n        setitimer(ITIMER_REAL, &v, NULL);\n}\n\nstatic void clear_progress_signal(void)\n{\n        struct itimerval v = {{0,},};\n        setitimer(ITIMER_REAL, &v, NULL);\n        signal(SIGALRM, SIG_IGN);\n        progress_update = 0;\n}\n\nSo it uses a 1 sec timeout, which is a good match to my implementation, but also uses it_interval, meant to 're-arm' the timer after\nit expired.\nMy implementation doesn't do that at all, and I also don't see how it possibly could (short of installing a signal handler, which\nthen conflicts with the one use in progress.c).\nOn top here SA_RESTART is used, which is not available in HP NonStop (so I have a \"-DSA_RESTART=0\" in COMPAT_CFLAGS).\n\nIn builtin/log.c it doesn't use it_interval, which is a good match to my implementation, but uses 1/2 a sec and 1/10 sec, so here\nwould be a victim of a 1 sec upgrade. This is probably acceptable.\n...\n         * NOTE! We don't use \"it_interval\", because if the\n         * reader isn't listening, we want our output to be\n         * throttled by the writing, and not have the timer\n         * trigger every second even if we're blocked on a\n         * reader!\n         */\n        early_output_timer.it_value.tv_sec = 0;\n        early_output_timer.it_value.tv_usec = 500000;\n        setitimer(ITIMER_REAL, &early_output_timer, NULL);\n...\nstatic void setup_early_output(struct rev_info *rev)\n{\n        struct sigaction sa;\n\n        /*\n         * Set up the signal handler, minimally intrusively:\n         * we only set a single volatile integer word (not\n         * using sigatomic_t - trying to avoid unnecessary\n         * system dependencies and headers), and using\n         * SA_RESTART.\n         */\n        memset(&sa, 0, sizeof(sa));\n        sa.sa_handler = early_output;\n        sigemptyset(&sa.sa_mask);\n        sa.sa_flags = SA_RESTART;\n        sigaction(SIGALRM, &sa, NULL);\n\n        /*\n         * If we can get the whole output in less than a\n         * tenth of a second, don't even bother doing the\n         * early-output thing..\n         *\n         * This is a one-time-only trigger.\n         */\n        early_output_timer.it_value.tv_sec = 0;\n        early_output_timer.it_value.tv_usec = 100000;\n        setitimer(ITIMER_REAL, &early_output_timer, NULL);\n}\n\nstatic void finish_early_output(struct rev_info *rev)\n{\n        int n = estimate_commit_count(rev, rev->commits);\n        signal(SIGALRM, SIG_IGN);\n        show_early_header(rev, \"done\", n);\n}\n\nThis means, however, at least to my understanding, that my setitimer() basically degrades to an 'alarm(1);' resp. 'alarm(0);', so\ncould possibly be simplified to:\n\n#ifdef NO_SETITIMER /* poor man's setitimer() */\n#define setitimer(w,v,o) alarm((v)->it_value.tv_sec+((v)->it_value.tv_usec>0))\n#endif\n\nin e.g. git-compat-util.h\n\nOpinions?\n\nBye, Jojo\n"},{"id":"198261","messageId":"5044F3DB.9060908@kdbg.org","threadId":"31343","inReplyTo":"000d01cd89b6$d5ba6c30$812f4490$@schmitz-digital.de","subject":"Re: [PATCH 1/2] Support for setitimer() on platforms lacking it","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2012-09-03T18:15:55Z","receivedAt":"2012-09-03T18:15:55Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 03.09.2012 11:31, schrieb Joachim Schmitz:\n> \n> Hmm, I see that there the errors are handled differently, like this:\n> \n>         if (ovalue != NULL)\n>                 return errno = EINVAL,\n>                         error(\"setitimer param 3 != NULL not implemented\");\n> \n> Should this be done in my setitimer() too? Or rather be left to the caller?\n> I tend to the later.\n\nThe error message is really just a reminder that the implementation is\nnot complete. Writing it here has the advantage that it is much more\naccurate than a generic \"invalid argument\" or \"operation not supported\"\nerror that the caller would be able to write.\n\n-- Hannes\n"},{"id":"198262","messageId":"7v627vexxv.fsf@alter.siamese.dyndns.org","threadId":"31343","inReplyTo":"5044F3DB.9060908@kdbg.org","subject":"Re: [PATCH 1/2] Support for setitimer() on platforms lacking it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-03T18:57:48Z","receivedAt":"2012-09-03T18:57:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> Am 03.09.2012 11:31, schrieb Joachim Schmitz:\n>> \n>> Hmm, I see that there the errors are handled differently, like this:\n>> \n>>         if (ovalue != NULL)\n>>                 return errno = EINVAL,\n>>                         error(\"setitimer param 3 != NULL not implemented\");\n>> \n>> Should this be done in my setitimer() too? Or rather be left to the caller?\n>> I tend to the later.\n>\n> The error message is really just a reminder that the implementation is\n> not complete. Writing it here has the advantage that it is much more\n> accurate than a generic \"invalid argument\" or \"operation not supported\"\n> error that the caller would be able to write.\n\nJoachim quoted irrelevant (to you) part and made comments on it, but\nthe issue I raised by Ccing you was about diagnosing NULL passed in\nnewvalue parameter, which Joachim's code did like this:\n\n    > int git_setitimer(int which, const struct itimerval *value,\n    > \t\t\t\tstruct itimerval *ovalue)\n    > {\n    > \tint ret = 0;\n    >\n    > \tif (!value ) {\n    > \t\terrno = EFAULT;\n    > \t\treturn -1;\n\n    EFAULT is good ;-)\n\n    The emulation in mingw.c 6072fc3 (Windows: Implement setitimer() and\n    sigaction()., 2007-11-13) may want to be tightened in a similar way.\n\nbut mingw.c doesn't seem to.\n"},{"id":"198263","messageId":"7v1uijexor.fsf@alter.siamese.dyndns.org","threadId":"31343","inReplyTo":"000d01cd89b6$d5ba6c30$812f4490$@schmitz-digital.de","subject":"Re: [PATCH 1/2] Support for setitimer() on platforms lacking it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-03T19:03:16Z","receivedAt":"2012-09-03T19:03:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Joachim Schmitz\" <jojo@schmitz-digital.de> writes:\n\n>> > \tif (!value ) {\n>> \n>> Style: space before ')'?\n>\n> Will fix.\n>  \n>> > \t\terrno = EFAULT;\n>> > \t\treturn -1;\n>> \n>> EFAULT is good ;-)\n>\n> That's what 'man setitimer()' on Linux says to happen if invalid value is found.\n>  \n>> The emulation in mingw.c 6072fc3 (Windows: Implement setitimer() and\n>> sigaction()., 2007-11-13) may want to be tightened in a similar way.\n>\n\n> Hmm, I see that there the errors are handled differently, like this:\n>\n>         if (ovalue != NULL)\n>                 return errno = EINVAL,\n>                         error(\"setitimer param 3 != NULL not implemented\");\n>\n> Should this be done in my setitimer() too? Or rather be left to the caller?\n> I tend to the later.\n\nI don't care too deeply either way.  The above was not a comment\nmeant for you, but was to point out the error checking when the\nnewvalue is NULL---it is missing in mingw.c and I think the\ncondition should be checked.\n\n> On top here SA_RESTART is used, which is not available in HP\n> NonStop (so I have a \"-DSA_RESTART=0\" in COMPAT_CFLAGS).\n\nIf you cannot re-trigger the timer, then you will see \"20%\" shown\nafter one second, silence for 4 seconds and then \"done\", for an\noperation that takes 5 seconds.  Which is not the end of the world,\nthough.  It does not affect correctness.\n\nThe other use of itimer in our codebase is the early-output timer,\nbut that also is about perceived latency, and not about correctness,\nso it is possible that you do not have to support anything (i.e. not\neven setting an alarm) at all.\n"},{"id":"198267","messageId":"003601cd8a0f$6a792840$3f6b78c0$@schmitz-digital.de","threadId":"31343","inReplyTo":"7v1uijexor.fsf@alter.siamese.dyndns.org","subject":"RE: [PATCH 1/2] Support for setitimer() on platforms lacking it","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-09-03T20:05:06Z","receivedAt":"2012-09-03T20:05:06Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"> From: Junio C Hamano [mailto:gitster@pobox.com]\n> Sent: Monday, September 03, 2012 9:03 PM\n> To: Joachim Schmitz\n> Cc: git@vger.kernel.org; 'Johannes Sixt'\n> Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it\n> \n> \"Joachim Schmitz\" <jojo@schmitz-digital.de> writes:\n> \n> >> > \tif (!value ) {\n> >>\n> >> Style: space before ')'?\n> >\n> > Will fix.\n> >\n> >> > \t\terrno = EFAULT;\n> >> > \t\treturn -1;\n> >>\n> >> EFAULT is good ;-)\n> >\n> > That's what 'man setitimer()' on Linux says to happen if invalid value is found.\n> >\n> >> The emulation in mingw.c 6072fc3 (Windows: Implement setitimer() and\n> >> sigaction()., 2007-11-13) may want to be tightened in a similar way.\n> >\n> \n> > Hmm, I see that there the errors are handled differently, like this:\n> >\n> >         if (ovalue != NULL)\n> >                 return errno = EINVAL,\n> >                         error(\"setitimer param 3 != NULL not implemented\");\n> >\n> > Should this be done in my setitimer() too? Or rather be left to the caller?\n> > I tend to the later.\n> \n> I don't care too deeply either way.  The above was not a comment\n> meant for you, but was to point out the error checking when the\n> newvalue is NULL---it is missing in mingw.c and I think the\n> condition should be checked.\n\n Ah, OK. Guess Johannes and I misunderstood ;-)\n\n> > On top here SA_RESTART is used, which is not available in HP\n> > NonStop (so I have a \"-DSA_RESTART=0\" in COMPAT_CFLAGS).\n> \n> If you cannot re-trigger the timer, then you will see \"20%\" shown\n> after one second, silence for 4 seconds and then \"done\", for an\n> operation that takes 5 seconds.  Which is not the end of the world,\n> though.  It does not affect correctness.\n\nThat does seem to work, if I do e.g. a \"git clone\" on git itself (being a fairly large repository), I see it updating the % values\nabout once per second.\n\n> The other use of itimer in our codebase is the early-output timer,\n> but that also is about perceived latency, and not about correctness,\n> so it is possible that you do not have to support anything (i.e. not\n> even setting an alarm) at all.\n\nOK, I'll go for that one-liner in git-compat-utils.h then\n\n#ifdef NO_SETITIMER /* poor man's setitimer() */\n#define setitimer(w,v,o) alarm((v)->it_value.tv_sec+((v)->it_value.tv_usec>0))\n#endif\n\nIt certainly seems to work just fine for me.\nCould as well be #ifdef __TANDEM, I won't mind.\n\nBye, Jojo\n"},{"id":"198310","messageId":"7vzk55bu8s.fsf@alter.siamese.dyndns.org","threadId":"31343","inReplyTo":"003601cd8a0f$6a792840$3f6b78c0$@schmitz-digital.de","subject":"Re: [PATCH 1/2] Support for setitimer() on platforms lacking it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-04T16:58:11Z","receivedAt":"2012-09-04T16:58:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Joachim Schmitz\" <jojo@schmitz-digital.de> writes:\n\n>> If you cannot re-trigger the timer, then you will see \"20%\" shown\n>> after one second, silence for 4 seconds and then \"done\", for an\n>> operation that takes 5 seconds.  Which is not the end of the world,\n>> though.  It does not affect correctness.\n>\n> That does seem to work, if I do e.g. a \"git clone\" on git itself\n> (being a fairly large repository), I see it updating the % values\n> about once per second.\n\nEhh, so somebody is re-arming the alarm().  I am not sure where,\nthough.\n\n ... thinks for a while, then a lightbulb slowly starts to glow ...\n\nWhere are you cloning from, and does the other side of the clone\n(i.e. upload-pack) also run on your tandem port?  If you are cloning\nfrom one of my public distribution points (e.g. k.org, repo.or.cz,\nor github.com), then I think the progress indicator you are seeing\nis coming from the other side, not generated by your local timer.\n\nOnly with the observation of \"clone\", I cannot tell if your timer is\nworking.  You can try repacking the test repository you created by\nyour earlier \"git clone\" with \"git repack -a -d -f\" and see what\nhappens.\n\n> OK, I'll go for that one-liner in git-compat-utils.h then\n>\n> #ifdef NO_SETITIMER /* poor man's setitimer() */\n> #define setitimer(w,v,o) alarm((v)->it_value.tv_sec+((v)->it_value.tv_usec>0))\n> #endif\n>\n> It certainly seems to work just fine for me.\n"},{"id":"198314","messageId":"002801cd8ac2$10937480$31ba5d80$@schmitz-digital.de","threadId":"31343","inReplyTo":"7vzk55bu8s.fsf@alter.siamese.dyndns.org","subject":"RE: [PATCH 1/2] Support for setitimer() on platforms lacking it","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-09-04T17:23:55Z","receivedAt":"2012-09-04T17:23:55Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"> From: Junio C Hamano [mailto:gitster@pobox.com]\n> Sent: Tuesday, September 04, 2012 6:58 PM\n> To: Joachim Schmitz\n> Cc: git@vger.kernel.org; 'Johannes Sixt'\n> Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it\n> \n> \"Joachim Schmitz\" <jojo@schmitz-digital.de> writes:\n> \n> >> If you cannot re-trigger the timer, then you will see \"20%\" shown\n> >> after one second, silence for 4 seconds and then \"done\", for an\n> >> operation that takes 5 seconds.  Which is not the end of the world,\n> >> though.  It does not affect correctness.\n> >\n> > That does seem to work, if I do e.g. a \"git clone\" on git itself\n> > (being a fairly large repository), I see it updating the % values\n> > about once per second.\n> \n> Ehh, so somebody is re-arming the alarm().  I am not sure where,\n> though.\n> \n>  ... thinks for a while, then a lightbulb slowly starts to glow ...\n> \n> Where are you cloning from, and does the other side of the clone\n> (i.e. upload-pack) also run on your tandem port?  If you are cloning\n> from one of my public distribution points (e.g. k.org, repo.or.cz,\n> or github.com), then I think the progress indicator you are seeing\n> is coming from the other side, not generated by your local timer.\n\nI used GutHub\nThe cloning from NonStop doesn't work at all, different story, but looks like poll isn#t working.\nNot poll's fault tough, but on out plaftom ssh (non-interactive) give a pipe rather than a socket and recv(...MSG_PEEK) then fails\nwith ENOTSOCK\n\n> Only with the observation of \"clone\", I cannot tell if your timer is\n> working.  You can try repacking the test repository you created by\n> your earlier \"git clone\" with \"git repack -a -d -f\" and see what\n> happens.\n\nIt does update the counter too.\n\n> > OK, I'll go for that one-liner in git-compat-utils.h then\n> >\n> > #ifdef NO_SETITIMER /* poor man's setitimer() */\n> > #define setitimer(w,v,o) alarm((v)->it_value.tv_sec+((v)->it_value.tv_usec>0))\n> > #endif\n> >\n> > It certainly seems to work just fine for me.\n\nBye, Jojo\n"},{"id":"198319","messageId":"7vwr09abim.fsf@alter.siamese.dyndns.org","threadId":"31343","inReplyTo":"002801cd8ac2$10937480$31ba5d80$@schmitz-digital.de","subject":"Re: [PATCH 1/2] Support for setitimer() on platforms lacking it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-04T18:28:01Z","receivedAt":"2012-09-04T18:28:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Joachim Schmitz\" <jojo@schmitz-digital.de> writes:\n\n>> Only with the observation of \"clone\", I cannot tell if your timer is\n>> working.  You can try repacking the test repository you created by\n>> your earlier \"git clone\" with \"git repack -a -d -f\" and see what\n>> happens.\n>\n> It does update the counter too.\n\nYeah, that was not a very good way to diagnose it.\n\nYou see the progress from pack-objects (which is the underlying\nmachinery \"git repack\" uses) only because it knows how many objects\nit is going to pack, and it updates the progress meter for every\nper-cent progress it makes, without any help from the timer\ninterrupt.\n"},{"id":"198321","messageId":"7vobllaami.fsf@alter.siamese.dyndns.org","threadId":"31343","inReplyTo":"7vwr09abim.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] Support for setitimer() on platforms lacking it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-04T18:47:17Z","receivedAt":"2012-09-04T18:47:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> \"Joachim Schmitz\" <jojo@schmitz-digital.de> writes:\n>\n>>> Only with the observation of \"clone\", I cannot tell if your timer is\n>>> working.  You can try repacking the test repository you created by\n>>> your earlier \"git clone\" with \"git repack -a -d -f\" and see what\n>>> happens.\n>>\n>> It does update the counter too.\n>\n> Yeah, that was not a very good way to diagnose it.\n>\n> You see the progress from pack-objects (which is the underlying\n> machinery \"git repack\" uses) only because it knows how many objects\n> it is going to pack, and it updates the progress meter for every\n> per-cent progress it makes, without any help from the timer\n> interrupt.\n\nI think the \"Counting objects: $number\" phase is purely driven by\nthe timer, as there is no way to say \"we are done X per-cent so\nfar\".\n\nDoesn't your repack show \"Counting objects: \" with a number once,\npause forever and then show \"Counting objects: $number, done.\"?\n"},{"id":"198322","messageId":"50464D0A.9050502@kdbg.org","threadId":"31343","inReplyTo":"002801cd8ac2$10937480$31ba5d80$@schmitz-digital.de","subject":"Re: [PATCH 1/2] Support for setitimer() on platforms lacking it","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2012-09-04T18:48:42Z","receivedAt":"2012-09-04T18:48:42Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 04.09.2012 19:23, schrieb Joachim Schmitz:\n>> From: Junio C Hamano [mailto:gitster@pobox.com]\n>> Only with the observation of \"clone\", I cannot tell if your timer is\n>> working.  You can try repacking the test repository you created by\n>> your earlier \"git clone\" with \"git repack -a -d -f\" and see what\n>> happens.\n> \n> It does update the counter too.\n\nLast time I looked at the progress code, it updated the progress text\nalso every time the percent value changed. If you have a large count or\nlarge objects such that it takes more than a second to increase the\npercentage, than you don't get a smooth progress. Nor do you for phases\nthat do not have a percentage, like the \"counting objects\" phase of\npack-objects.\n\n-- Hannes\n"},{"id":"198345","messageId":"002c01cd8ae6$f23d7ec0$d6b87c40$@schmitz-digital.de","threadId":"31343","inReplyTo":"7vobllaami.fsf@alter.siamese.dyndns.org","subject":"RE: [PATCH 1/2] Support for setitimer() on platforms lacking it","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-09-04T21:47:56Z","receivedAt":"2012-09-04T21:47:56Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"> From: Junio C Hamano [mailto:gitster@pobox.com]\n> Sent: Tuesday, September 04, 2012 8:47 PM\n> To: Joachim Schmitz\n> Cc: git@vger.kernel.org; 'Johannes Sixt'\n> Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it\n> \n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > \"Joachim Schmitz\" <jojo@schmitz-digital.de> writes:\n> >\n> >>> Only with the observation of \"clone\", I cannot tell if your timer is\n> >>> working.  You can try repacking the test repository you created by\n> >>> your earlier \"git clone\" with \"git repack -a -d -f\" and see what\n> >>> happens.\n> >>\n> >> It does update the counter too.\n> >\n> > Yeah, that was not a very good way to diagnose it.\n> >\n> > You see the progress from pack-objects (which is the underlying\n> > machinery \"git repack\" uses) only because it knows how many objects\n> > it is going to pack, and it updates the progress meter for every\n> > per-cent progress it makes, without any help from the timer\n> > interrupt.\n> \n> I think the \"Counting objects: $number\" phase is purely driven by\n> the timer, as there is no way to say \"we are done X per-cent so\n> far\".\n> \n> Doesn't your repack show \"Counting objects: \" with a number once,\n> pause forever and then show \"Counting objects: $number, done.\"?\n\nYes, only once, when it is done\n$ ./git repack -a -d -f\nwarning: no threads support, ignoring --threads\nCounting objects: 140302, done.\nCompressing objects:   1% (1385/138407)\n"},{"id":"198347","messageId":"7v627t8l2d.fsf@alter.siamese.dyndns.org","threadId":"31343","inReplyTo":"002c01cd8ae6$f23d7ec0$d6b87c40$@schmitz-digital.de","subject":"Re: [PATCH 1/2] Support for setitimer() on platforms lacking it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-04T22:44:42Z","receivedAt":"2012-09-04T22:44:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Joachim Schmitz\" <jojo@schmitz-digital.de> writes:\n\n>> From: Junio C Hamano [mailto:gitster@pobox.com]\n>> Sent: Tuesday, September 04, 2012 8:47 PM\n>> To: Joachim Schmitz\n>> Cc: git@vger.kernel.org; 'Johannes Sixt'\n>> Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it\n>> \n>> Junio C Hamano <gitster@pobox.com> writes:\n>> \n>> > \"Joachim Schmitz\" <jojo@schmitz-digital.de> writes:\n>> >\n>> >>> Only with the observation of \"clone\", I cannot tell if your timer is\n>> >>> working.  You can try repacking the test repository you created by\n>> >>> your earlier \"git clone\" with \"git repack -a -d -f\" and see what\n>> >>> happens.\n>> >>\n>> >> It does update the counter too.\n>> >\n>> > Yeah, that was not a very good way to diagnose it.\n>> >\n>> > You see the progress from pack-objects (which is the underlying\n>> > machinery \"git repack\" uses) only because it knows how many objects\n>> > it is going to pack, and it updates the progress meter for every\n>> > per-cent progress it makes, without any help from the timer\n>> > interrupt.\n>> \n>> I think the \"Counting objects: $number\" phase is purely driven by\n>> the timer, as there is no way to say \"we are done X per-cent so\n>> far\".\n>> \n>> Doesn't your repack show \"Counting objects: \" with a number once,\n>> pause forever and then show \"Counting objects: $number, done.\"?\n>\n> Yes, only once, when it is done\n> $ ./git repack -a -d -f\n> warning: no threads support, ignoring --threads\n> Counting objects: 140302, done.\n> Compressing objects:   1% (1385/138407)\n\nSo this strongly suggests that (1) your \"poor-man's\" is not a real\nsubstitute for recurring itimer, and (2) users could live with the\nprogress.c code without any itimer firing.\n\nPerhaps a no-op macro would work equally well?\n"},{"id":"198365","messageId":"00a601cd8b4d$2986dfa0$7c949ee0$@schmitz-digital.de","threadId":"31343","inReplyTo":"7v627t8l2d.fsf@alter.siamese.dyndns.org","subject":"RE: [PATCH 1/2] Support for setitimer() on platforms lacking it","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-09-05T09:59:37Z","receivedAt":"2012-09-05T09:59:37Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"> From: Junio C Hamano [mailto:gitster@pobox.com]\n> Sent: Wednesday, September 05, 2012 12:45 AM\n> To: Joachim Schmitz\n> Cc: git@vger.kernel.org; 'Johannes Sixt'\n> Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it\n> \n> \"Joachim Schmitz\" <jojo@schmitz-digital.de> writes:\n> \n> >> From: Junio C Hamano [mailto:gitster@pobox.com]\n> >> Sent: Tuesday, September 04, 2012 8:47 PM\n> >> To: Joachim Schmitz\n> >> Cc: git@vger.kernel.org; 'Johannes Sixt'\n> >> Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it\n> >>\n> >> Junio C Hamano <gitster@pobox.com> writes:\n> >>\n> >> > \"Joachim Schmitz\" <jojo@schmitz-digital.de> writes:\n> >> >\n> >> >>> Only with the observation of \"clone\", I cannot tell if your timer is\n> >> >>> working.  You can try repacking the test repository you created by\n> >> >>> your earlier \"git clone\" with \"git repack -a -d -f\" and see what\n> >> >>> happens.\n> >> >>\n> >> >> It does update the counter too.\n> >> >\n> >> > Yeah, that was not a very good way to diagnose it.\n> >> >\n> >> > You see the progress from pack-objects (which is the underlying\n> >> > machinery \"git repack\" uses) only because it knows how many objects\n> >> > it is going to pack, and it updates the progress meter for every\n> >> > per-cent progress it makes, without any help from the timer\n> >> > interrupt.\n> >>\n> >> I think the \"Counting objects: $number\" phase is purely driven by\n> >> the timer, as there is no way to say \"we are done X per-cent so\n> >> far\".\n> >>\n> >> Doesn't your repack show \"Counting objects: \" with a number once,\n> >> pause forever and then show \"Counting objects: $number, done.\"?\n> >\n> > Yes, only once, when it is done\n> > $ ./git repack -a -d -f\n> > warning: no threads support, ignoring --threads\n> > Counting objects: 140302, done.\n> > Compressing objects:   1% (1385/138407)\n> \n> So this strongly suggests that (1) your \"poor-man's\" is not a real\n> substitute for recurring itimer, and (2) users could live with the\n> progress.c code without any itimer firing.\n\nOK\n\n> Perhaps a no-op macro would work equally well?\n\nLike the following:\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 18089f0..55b9421 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h@@ -163,6 +163,10 @@\n #define probe_utf8_pathname_composition(a,b)\n #endif\n\n+#ifdef NO_SETITIMER\n+#define setitimer(w,v,o) /* NOP */\n+#endif\n+\n #ifdef MKDIR_WO_TRAILING_SLASH\n #define mkdir(a,b) compat_mkdir_wo_trailing_slash((a),(b))\n extern int compat_mkdir_wo_trailing_slash(const char*, mode_t);\n\nDoes work for me and does not seem to make any difference, not in those test cases at least\n\nDoes the inability to re-arm the timer depend on SA_RESTART, possibly?\nIf so we may instead want \n#if SA_RSTART == 0  && defined(NO_SETITIMER)\n\nBye, Jojo\n"}]}