upload-pack: keep poll(2)'s timeout to -1
PATCH 1/1 upload-pack: keep poll(2)'s timeout to -1
Keep poll's timeout at -1 when uploadpack.keepalive = 0, instead of setting it to -1000, since some pedantic old systems (eg HP-UX) and the gnulib compat/poll will treat only -1 as the valid value for an infinite timeout.
--- upload-pack.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
upload-pack.c +3 -1
@@ -167,7 +167,9 @@ static void create_pack_file(void)if (!pollsize)break ; - ret =poll (pfd, pollsize,1000 * keepalive); + ret =poll (pfd, pollsize, + keepalive <0 ?-1 :1000 * keepalive); +if (ret <0 ) {if (errno !=EINTR ) { error("poll failed, resuming: %s",
Signature · 1 line
1.7.10.4
Quoted 4 lines · Edward Thomson
Keep poll's timeout at -1 when uploadpack.keepalive = 0, instead of setting it to -1000, since some pedantic old systems (eg HP-UX) and the gnulib compat/poll will treat only -1 as the valid value for an infinite timeout.
That makes sense, and POSIX only specifies the behavior for -1 anyway. The patch itself looks obviously correct. Thanks.
Since we're now translating the keepalive value, and since there's no way to set it to "0" (nor would that really have any meaning), I guess we could switch the internal "no keepalive" value to 0, and do:
ret = poll(pfd, pollsize, keepalive ? 1000 * keepalive : -1);
which would let us avoid setting it to -1 in some other spots. I dunno if that actually makes a real difference to maintainability, though. Either way:
-Peff
Quoted 16 lines · Jeff King
Quoted 4 lines · Edward Thomson
Keep poll's timeout at -1 when uploadpack.keepalive = 0, instead of setting it to -1000, since some pedantic old systems (eg HP-UX) and the gnulib compat/poll will treat only -1 as the valid value for an infinite timeout.
That makes sense, and POSIX only specifies the behavior for -1 anyway. The patch itself looks obviously correct. Thanks.
Since we're now translating the keepalive value, and since there's no way to set it to "0" (nor would that really have any meaning), I guess we could switch the internal "no keepalive" value to 0, and do:
ret = poll(pfd, pollsize, keepalive ? 1000 * keepalive : -1);
Quoted 7 lines
which would let us avoid setting it to -1 in some other spots. I dunno if that actually makes a real difference to maintainability, though. Either way:
Acked-by Jeff King <[email protected]>-Peff
There is 1000 * wakeup in credential-cache--daemon.c, by the way.
Quoted 1 line · Junio C Hamano
There is 1000 * wakeup in
credential-cache--daemon.c, by the way.
Ah, nevermind. That uses an expiration computed, not some "we can choose to block indefinitely" configuration.
Quoted 8 lines · Jeff King
Since we're now translating the keepalive value, and since there's no way to set it to "0" (nor would that really have any meaning), I guess we could switch the internal "no keepalive" value to 0, and do:
ret = poll(pfd, pollsize, keepalive ? 1000 * keepalive : -1);which would let us avoid setting it to -1 in some other spots. I dunno if that actually makes a real difference to maintainability, though.
Where we parse and set the value of the variable, we do this:
else if (!strcmp("uploadpack.keepalive", var)) {
keepalive = git_config_int(var, value);
if (!keepalive)
keepalive = -1;
}The condition may have to become "if (keepalive <= 0)".
Quoted 5 lines
Either way:
Acked-by Jeff King <[email protected]>-Peff
Yeah, either way, the patch as-posted is good. Thanks.
Quoted 13 lines · Junio C Hamano
Quoted 8 lines · Jeff King
Since we're now translating the keepalive value, and since there's no way to set it to "0" (nor would that really have any meaning), I guess we could switch the internal "no keepalive" value to 0, and do:
ret = poll(pfd, pollsize, keepalive ? 1000 * keepalive : -1);which would let us avoid setting it to -1 in some other spots. I dunno if that actually makes a real difference to maintainability, though.
Where we parse and set the value of the variable, we do this:
Quoted 7 lines
else if (!strcmp("uploadpack.keepalive", var)) { keepalive = git_config_int(var, value); if (!keepalive) keepalive = -1; }The condition may have to become "if (keepalive <= 0)".
Yeah, I wasn't thinking we would get negative values from the user (we don't document them at all), but we should probably do something sensible. Let's just leave it at Ed's patch.
-Peff
Quoted 4 lines · Jeff King
Yeah, I wasn't thinking we would get negative values from the user (we don't document them at all), but we should probably do something sensible. Let's just leave it at Ed's patch.
Thanks, both. Apologies for the dumb question: is there anything additional that I need to do (repost with your Acked-by, for example) or is this adequate as-is?
Thanks-
-ed
Quoted 9 lines · Edward Thomson
Quoted 4 lines · Jeff King
Yeah, I wasn't thinking we would get negative values from the user (we don't document them at all), but we should probably do something sensible. Let's just leave it at Ed's patch.
Thanks, both. Apologies for the dumb question: is there anything additional that I need to do (repost with your Acked-by, for example) or is this adequate as-is?
I've picked it up and queued it on 'pu'. Thanks.
commit 6c71f8b0d3d39beffe050f92f33a25dc30dffca3
Author: Edward Thomson <[email protected]> Date: Fri Aug 22 15:19:11 2014 +0000
upload-pack: keep poll(2)'s timeout to -1
Keep poll's timeout at -1 when uploadpack.keepalive = 0, instead of setting it to -1000, since some pedantic old systems (eg HP-UX) and the gnulib compat/poll will treat only -1 as the valid value for an infinite timeout.