# [PATCH] upload-pack: keep poll(2)'s timeout to -1 8 messages from 2014-08-22 to 2014-08-22. Participants: Edward Thomson, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/37399 ## Edward Thomson, 2014-08-22 15:19 Subject: [PATCH] upload-pack: keep poll(2)'s timeout to -1 Message-ID: <20140822151911.GA8531@debian> URL: https://gitlist.dev/e/20140822151911.GA8531%40debian ``` 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. Signed-off-by: Edward Thomson --- upload-pack.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/upload-pack.c b/upload-pack.c index 01de944..433211a 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -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", -- 1.7.10.4 ``` ## Jeff King, 2014-08-22 15:44 Subject: Re: [PATCH] upload-pack: keep poll(2)'s timeout to -1 Message-ID: <20140822154445.GA19135@peff.net> URL: https://gitlist.dev/e/20140822154445.GA19135%40peff.net In-Reply-To: <20140822151911.GA8531@debian> ``` On Fri, Aug 22, 2014 at 03:19:11PM +0000, Edward Thomson wrote: > 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: Acked-by: Jeff King -Peff ``` ## Junio C Hamano, 2014-08-22 15:56 Subject: Re: [PATCH] upload-pack: keep poll(2)'s timeout to -1 Message-ID: URL: https://gitlist.dev/e/xmqqr408plgj.fsf%40gitster.dls.corp.google.com In-Reply-To: <20140822154445.GA19135@peff.net> ``` Jeff King writes: > 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)". > Either way: > > Acked-by: Jeff King > > -Peff Yeah, either way, the patch as-posted is good. Thanks. ``` ## Jeff King, 2014-08-22 16:03 Subject: Re: [PATCH] upload-pack: keep poll(2)'s timeout to -1 Message-ID: <20140822160334.GA20789@peff.net> URL: https://gitlist.dev/e/20140822160334.GA20789%40peff.net In-Reply-To: ``` On Fri, Aug 22, 2014 at 08:56:12AM -0700, Junio C Hamano wrote: > Jeff King writes: > > > 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)". 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 ``` ## Edward Thomson, 2014-08-22 16:27 Subject: Re: [PATCH] upload-pack: keep poll(2)'s timeout to -1 Message-ID: <20140822162711.GA8598@debian> URL: https://gitlist.dev/e/20140822162711.GA8598%40debian In-Reply-To: <20140822160334.GA20789@peff.net> ``` On Fri, Aug 22, 2014 at 12:03:34PM -0400, Jeff King wrote: > > 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 ``` ## Junio C Hamano, 2014-08-22 18:19 Subject: Re: [PATCH] upload-pack: keep poll(2)'s timeout to -1 Message-ID: URL: https://gitlist.dev/e/xmqq1ts8peud.fsf%40gitster.dls.corp.google.com In-Reply-To: <20140822154445.GA19135@peff.net> ``` Jeff King writes: > On Fri, Aug 22, 2014 at 03:19:11PM +0000, Edward Thomson wrote: > >> 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: > > Acked-by: Jeff King > > -Peff There is 1000 * wakeup in credential-cache--daemon.c, by the way. ``` ## Junio C Hamano, 2014-08-22 18:21 Subject: Re: [PATCH] upload-pack: keep poll(2)'s timeout to -1 Message-ID: URL: https://gitlist.dev/e/xmqqwqa0o05g.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Junio C Hamano writes: > 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. ``` ## Junio C Hamano, 2014-08-22 18:26 Subject: Re: [PATCH] upload-pack: keep poll(2)'s timeout to -1 Message-ID: URL: https://gitlist.dev/e/xmqqsikonzx0.fsf%40gitster.dls.corp.google.com In-Reply-To: <20140822162711.GA8598@debian> ``` Edward Thomson writes: > On Fri, Aug 22, 2014 at 12:03:34PM -0400, Jeff King wrote: >> >> 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 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. Signed-off-by: Edward Thomson Acked-by: Jeff King Signed-off-by: Junio C Hamano ```