[PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

[PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

[Johannes Sixt]

From: Johannes Sixt <johannes.sixt@telecom.at>

How come we got along with this not very portable construct for so long?
Probably because the array sizes were computed from the results of
strlen() of string constants. Anyway, a follow-up patch will make the
lengths really non-constant.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 sideband.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/sideband.c b/sideband.c
index 756bbc2..513d7b3 100644
--- a/sideband.c
+++ b/sideband.c
@@ -19,7 +19,10 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 {
 	unsigned pf = strlen(PREFIX);
 	unsigned sf = strlen(SUFFIX);
-	char buf[pf + LARGE_PACKET_MAX + sf + 1];
+	char *buf, *save;
+
+	save = xmalloc(sf);
+	buf = xmalloc(pf + LARGE_PACKET_MAX + sf + 1);
 	memcpy(buf, PREFIX, pf);
 	while (1) {
 		int band, len;
@@ -29,6 +32,8 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 		if (len < 1) {
 			len = sprintf(buf, "%s: protocol error: no band designator\n", me);
 			safe_write(err, buf, len);
+			free(buf);
+			free(save);
 			return SIDEBAND_PROTOCOL_ERROR;
 		}
 		band = buf[pf] & 0xff;
@@ -38,6 +43,8 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 			buf[pf] = ' ';
 			buf[pf+1+len] = '\n';
 			safe_write(err, buf, pf+1+len+1);
+			free(buf);
+			free(save);
 			return SIDEBAND_REMOTE_ERROR;
 		case 2:
 			buf[pf] = ' ';
@@ -59,7 +66,6 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 				 * line data actually contains something.
 				 */
 				if (brk > pf+1 + 1) {
-					char save[sf];
 					memcpy(save, buf + brk, sf);
 					buf[brk + sf - 1] = buf[brk - 1];
 					memcpy(buf + brk - 1, SUFFIX, sf);
@@ -83,9 +89,13 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 				      "%s: protocol error: bad band #%d\n",
 				      me, band);
 			safe_write(err, buf, len);
+			free(buf);
+			free(save);
 			return SIDEBAND_PROTOCOL_ERROR;
 		}
 	}
+	free(buf);
+	free(save);
 	return 0;
 }

-- 
1.5.4.rc2.815.g2f849-dirty

Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

[Nicolas Pitre]

On Tue, 8 Jan 2008, Johannes Sixt wrote:

> From: Johannes Sixt <johannes.sixt@telecom.at>
> 
> How come we got along with this not very portable construct for so long?
> Probably because the array sizes were computed from the results of
> strlen() of string constants.

Maybe because it isn't not so unportable anymore?  I doubt that 
compilers that don't know about automatic arrays would be smart enough 
to notice the variable was actually a constant due to the strlen() of a 
constant string and just do like if there wasn't any variable for the 
array size.

> Anyway, a follow-up patch will make the
> lengths really non-constant.

Fair enough.


Nicolas

Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

[Johannes Sixt]

Nicolas Pitre schrieb:
> On Tue, 8 Jan 2008, Johannes Sixt wrote:
>> How come we got along with this not very portable construct for so long?
>> Probably because the array sizes were computed from the results of
>> strlen() of string constants.
> 
> Maybe because it isn't not so unportable anymore?  I doubt that 
> compilers that don't know about automatic arrays would be smart enough 
> to notice the variable was actually a constant due to the strlen() of a 
> constant string and just do like if there wasn't any variable for the 
> array size.

I just tried it with Visual Age 6, and got this:

    CC sideband.o
"sideband.c", line 22.18: 1506-195 (S) Integral constant expression with a
value greater than zero is required.
"sideband.c", line 62.51: 1506-195 (S) Integral constant expression with a
value greater than zero is required.
make: *** [sideband.o] Error 1

But before I got to this point I had to change all 'static inline' in
git-compat-util.h to plain 'static'. So this compiler is out of the game
anyway.

Having said that, I'd actually prefer to stay with variable-sized arrays
if they prove portable enough because we don't need the handful of free()s
on function exits. Junio, if you like I can resend patch 2/2 using
variable-sized arrays.

-- Hannes

Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Having said that, I'd actually prefer to stay with variable-sized arrays
> if they prove portable enough because we don't need the handful of free()s
> on function exits. Junio, if you like I can resend patch 2/2 using
> variable-sized arrays.

As an old fashoned git myself, and given the fact that the
possible prefix and suffix are small number of short constant
strings, I actually prefer a simpler-and-more-stupid approach.

 sideband.c |   18 +++++++++++++-----
 1 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/sideband.c b/sideband.c
index 756bbc2..24f7f12 100644
--- a/sideband.c
+++ b/sideband.c
@@ -13,14 +13,22 @@
  */
 
 #define PREFIX "remote:"
-#define SUFFIX "\033[K"  /* change to "        " if ANSI sequences don't work */
 
 int recv_sideband(const char *me, int in_stream, int out, int err)
 {
 	unsigned pf = strlen(PREFIX);
-	unsigned sf = strlen(SUFFIX);
-	char buf[pf + LARGE_PACKET_MAX + sf + 1];
+	unsigned sf;
+	char buf[LARGE_PACKET_MAX + 100]; /* for marker slop */
+	char *suffix, *term;
+
 	memcpy(buf, PREFIX, pf);
+	term = getenv("TERM");
+	if (term && strcmp(term, "dumb"))
+		suffix = "\033[K";
+	else
+		suffix = "        ";
+	sf = strlen(suffix);
+
 	while (1) {
 		int band, len;
 		len = packet_read_line(in_stream, buf + pf, LARGE_PACKET_MAX);
@@ -59,10 +67,10 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 				 * line data actually contains something.
 				 */
 				if (brk > pf+1 + 1) {
-					char save[sf];
+					char save[100]; /* enough slop */
 					memcpy(save, buf + brk, sf);
 					buf[brk + sf - 1] = buf[brk - 1];
-					memcpy(buf + brk - 1, SUFFIX, sf);
+					memcpy(buf + brk - 1, suffix, sf);
 					safe_write(err, buf, brk + sf);
 					memcpy(buf + brk, save, sf);
 				} else

Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

[Johannes Sixt]

Junio C Hamano schrieb:
> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
>> Having said that, I'd actually prefer to stay with variable-sized arrays
>> if they prove portable enough because we don't need the handful of free()s
>> on function exits. Junio, if you like I can resend patch 2/2 using
>> variable-sized arrays.
> 
> As an old fashoned git myself, and given the fact that the
> possible prefix and suffix are small number of short constant
> strings, I actually prefer a simpler-and-more-stupid approach.
> 
>  sideband.c |   18 +++++++++++++-----
>  1 files changed, 13 insertions(+), 5 deletions(-)

Thanks, your version works well here, too, as a replacement of my two patches.

-- Hannes

Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

[Nicolas Pitre]

On Tue, 8 Jan 2008, Junio C Hamano wrote:

> As an old fashoned git myself, and given the fact that the
> possible prefix and suffix are small number of short constant
> strings, I actually prefer a simpler-and-more-stupid approach.

OK.  However I think the following on top of your patch would look 
cleaner:

diff --git a/sideband.c b/sideband.c
index 91e462c..b677781 100644
--- a/sideband.c
+++ b/sideband.c
@@ -14,19 +14,24 @@
 
 #define PREFIX "remote:"
 
+#define ANSI_SUFFIX "\033[K"
+#define DUMB_SUFFIX "        "
+
+#define FIX_SIZE 10  /* large enough for any of the above */
+
 int recv_sideband(const char *me, int in_stream, int out, int err)
 {
 	unsigned pf = strlen(PREFIX);
 	unsigned sf;
-	char buf[LARGE_PACKET_MAX + 100]; /* for marker slop */
+	char buf[LARGE_PACKET_MAX + 2*FIX_SIZE];
 	char *suffix, *term;
 
 	memcpy(buf, PREFIX, pf);
 	term = getenv("TERM");
 	if (term && strcmp(term, "dumb"))
-		suffix = "\033[K";
+		suffix = ANSI_SUFFIX;
 	else
-		suffix = "        ";
+		suffix = DUMB_SUFFIX;
 	sf = strlen(suffix);
 
 	while (1) {
@@ -67,7 +72,7 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 				 * line data actually contains something.
 				 */
 				if (brk > pf+1 + 1) {
-					char save[100]; /* enough slop */
+					char save[FIX_SIZE];
 					memcpy(save, buf + brk, sf);
 					buf[brk + sf - 1] = buf[brk - 1];
 					memcpy(buf + brk - 1, suffix, sf);

Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> From: Johannes Sixt <johannes.sixt@telecom.at>
>
> How come we got along with this not very portable construct for so long?
> Probably because the array sizes were computed from the results of
> strlen() of string constants. Anyway, a follow-up patch will make the
> lengths really non-constant.
>
> Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
> ---
>  sideband.c |   14 ++++++++++++--
>  1 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/sideband.c b/sideband.c
> index 756bbc2..513d7b3 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -19,7 +19,10 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
>  {
>  	unsigned pf = strlen(PREFIX);
>  	unsigned sf = strlen(SUFFIX);
> -	char buf[pf + LARGE_PACKET_MAX + sf + 1];
> +	char *buf, *save;
> +
> +	save = xmalloc(sf);
> +	buf = xmalloc(pf + LARGE_PACKET_MAX + sf + 1);

I have to wonder if the malloc() overhead is small enough
compared to the network bandwidth to make a two malloc-free
pairs per packet a non-issue...

Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

[Nicolas Pitre]

On Tue, 8 Jan 2008, Junio C Hamano wrote:

> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
> > From: Johannes Sixt <johannes.sixt@telecom.at>
> >
> > How come we got along with this not very portable construct for so long?
> > Probably because the array sizes were computed from the results of
> > strlen() of string constants. Anyway, a follow-up patch will make the
> > lengths really non-constant.
> >
> > Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
> > ---
> >  sideband.c |   14 ++++++++++++--
> >  1 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/sideband.c b/sideband.c
> > index 756bbc2..513d7b3 100644
> > --- a/sideband.c
> > +++ b/sideband.c
> > @@ -19,7 +19,10 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
> >  {
> >  	unsigned pf = strlen(PREFIX);
> >  	unsigned sf = strlen(SUFFIX);
> > -	char buf[pf + LARGE_PACKET_MAX + sf + 1];
> > +	char *buf, *save;
> > +
> > +	save = xmalloc(sf);
> > +	buf = xmalloc(pf + LARGE_PACKET_MAX + sf + 1);
> 
> I have to wonder if the malloc() overhead is small enough
> compared to the network bandwidth to make a two malloc-free
> pairs per packet a non-issue...

Eeek.  Overhead might be insignificant, but it still doesn't feel right.

What about using alloca() instead?  This is not like if we are doing 
funky things with the allocated memory anyway.


Nicolas

Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

[Junio C Hamano]

Nicolas Pitre <nico@cam.org> writes:

> On Tue, 8 Jan 2008, Junio C Hamano wrote:
>
>> Johannes Sixt <j.sixt@viscovery.net> writes:
>> 
>> > From: Johannes Sixt <johannes.sixt@telecom.at>
>> >
>> > How come we got along with this not very portable construct for so long?
>> > Probably because the array sizes were computed from the results of
>> > strlen() of string constants. Anyway, a follow-up patch will make the
>> > lengths really non-constant.
>> >
>> > Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
>> > ---
>> >  sideband.c |   14 ++++++++++++--
>> >  1 files changed, 12 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/sideband.c b/sideband.c
>> > index 756bbc2..513d7b3 100644
>> > --- a/sideband.c
>> > +++ b/sideband.c
>> > @@ -19,7 +19,10 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
>> >  {
>> >  	unsigned pf = strlen(PREFIX);
>> >  	unsigned sf = strlen(SUFFIX);
>> > -	char buf[pf + LARGE_PACKET_MAX + sf + 1];
>> > +	char *buf, *save;
>> > +
>> > +	save = xmalloc(sf);
>> > +	buf = xmalloc(pf + LARGE_PACKET_MAX + sf + 1);
>> 
>> I have to wonder if the malloc() overhead is small enough
>> compared to the network bandwidth to make a two malloc-free
>> pairs per packet a non-issue...
>
> Eeek.  Overhead might be insignificant, but it still doesn't feel right.
>
> What about using alloca() instead?  This is not like if we are doing 
> funky things with the allocated memory anyway.

That's double Eek as I recall AIX is not dead.

How about using a constant large enough slop?  It is not like
PREFIX and SUFFIX are different vastly between calls.

Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

[Nicolas Pitre]

On Tue, 8 Jan 2008, Junio C Hamano wrote:

> How about using a constant large enough slop?  It is not like
> PREFIX and SUFFIX are different vastly between calls.

The size of the buffer is certainly bounded since the prefix length is 
constant, the buffer size is constant, and both possible suffixes are 
also constants.


Nicolas

Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

[Johannes Sixt]

Junio C Hamano schrieb:
> Johannes Sixt <j.sixt@viscovery.net> writes:
>> @@ -19,7 +19,10 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
>>  {
>>  	unsigned pf = strlen(PREFIX);
>>  	unsigned sf = strlen(SUFFIX);
>> -	char buf[pf + LARGE_PACKET_MAX + sf + 1];
>> +	char *buf, *save;
>> +
>> +	save = xmalloc(sf);
>> +	buf = xmalloc(pf + LARGE_PACKET_MAX + sf + 1);
> 
> I have to wonder if the malloc() overhead is small enough
> compared to the network bandwidth to make a two malloc-free
> pairs per packet a non-issue...

recv_sideband() is called _once_per_connection_ and not for each packet.
Hence, these two mallocs should not concern us.

-- Hannes