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

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

[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.

Signed-off-by: Edward Thomson <ethomson@microsoft.com>
---
 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

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

[Jeff King]

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

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

[Junio C Hamano]

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)".

> Either way:
>
>   Acked-by: Jeff King <peff@peff.net>
>
> -Peff

Yeah, either way, the patch as-posted is good.  Thanks.

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

[Jeff King]

On Fri, Aug 22, 2014 at 08:56:12AM -0700, Junio C Hamano wrote:

> 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

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

[Edward Thomson]

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

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

[Junio C Hamano]

Edward Thomson <ethomson@edwardthomson.com> 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 <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>

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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@peff.net>
>
> -Peff

There is 1000 * wakeup in credential-cache--daemon.c, by the way.

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

[Junio C Hamano]

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.