threads / patch / 37399

patchupload-pack: keep poll(2)'s timeout to -1

Subject: [PATCH] upload-pack: keep poll(2)'s timeout to -1

## tl;dr

8 messages between Aug 22, 2014 and Aug 22, 2014. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Edward Thomson· Aug 22, 2014, 15:19 UTC · lore

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 <ethomson@microsoft.com>
---
 upload-pack.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to upload-pack.c +3 −1
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· Aug 22, 2014, 15:44 UTC · re: Edward Thomson · lore

Re: [PATCH] upload-pack: keep poll(2)'s timeout to -1

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@peff.net>
-Peff
Junio C Hamano· Aug 22, 2014, 15:56 UTC · re: Jeff King · lore

Re: [PATCH] upload-pack: keep poll(2)'s timeout to -1

Jeff King <peff@peff.net> writes:
Show 8 quoted lines
> 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)".
Show 5 quoted lines
> Either way:
>
>   Acked-by: Jeff King <peff@peff.net>
>
> -Peff
Yeah, either way, the patch as-posted is good.  Thanks.
Jeff King· Aug 22, 2014, 16:03 UTC · re: Junio C Hamano · lore

Re: [PATCH] upload-pack: keep poll(2)'s timeout to -1

On Fri, Aug 22, 2014 at 08:56:12AM -0700, Junio C Hamano wrote:
Show 20 quoted lines
> Jeff King <peff@peff.net> 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· Aug 22, 2014, 16:27 UTC · re: Jeff King · lore

Re: [PATCH] upload-pack: keep poll(2)'s timeout to -1

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· Aug 22, 2014, 18:26 UTC · re: Edward Thomson · lore

Re: [PATCH] upload-pack: keep poll(2)'s timeout to -1

Edward Thomson <ethomson@edwardthomson.com> writes:
Show 9 quoted lines
> 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 <ethomson@edwardthomson.com>
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 <ethomson@microsoft.com>
    Acked-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano· Aug 22, 2014, 18:19 UTC · re: Jeff King · lore

Re: [PATCH] upload-pack: keep poll(2)'s timeout to -1

Jeff King <peff@peff.net> writes:
Show 23 quoted lines
> 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@peff.net>
>
> -Peff
There is 1000 * wakeup in credential-cache--daemon.c, by the way.
Junio C Hamano· Aug 22, 2014, 18:21 UTC · re: Junio C Hamano · lore

Re: [PATCH] upload-pack: keep poll(2)'s timeout to -1

Junio C Hamano <gitster@pobox.com> 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.

← back to recent threads