Re: [PATCH] bisect: save heap memory. allocate only the required amount

[Jeff King <peff@peff.net>]

On Sun, Aug 24, 2014 at 04:39:37PM -0700, Junio C Hamano wrote:

> On Sun, Aug 24, 2014 at 8:10 AM, Stefan Beller <stefanbeller@gmail.com> wrote:
> >>       for (p = list, i = 0; i < cnt; i++) {
> >> -             struct name_decoration *r = xmalloc(sizeof(*r) + 100);
> >> +             char name[100];
> >
> > Would it make sense to convert the 'name' into a git strbuf?
> > Please have a look at Documentation/technical/api-strbuf.txt
> 
> Why not go one step further and format it twice, once only
> to measure the necessary size to allocate, allocate and
> then format into it for real? Then you do not need to print
> into a temporary strbuf, copy the result and free the strbuf,
> no?
> 
> BUT.
> 
> The string will always be "dist=" followed by decimal representation of
> a count that fits in "int" anyway, so I actually think use of strbuf is way
> overkill (and formatting it twice also is); the patch as posted should be
> just fine.

I think you are right, and the patch is the right direction (assuming we
want to do this; I question whether there are enough elements in the
list for us to care about the size, and if there are, we are probably
better off storing the int and formatting the strings on the fly).

I wonder if there is a way we could get rid of the magic "100" here,
though. Its meaning is "enough to hold 'dist=' and any integer". But you
have to read carefully to see that this call to sprintf is not a buffer
overflow. A strbuf is one way to get rid of it, though it is awkward
because we then have to copy the result into a flex-array structure.

It would be nice if there was some way to abstract the idea of
formatting a buffer directly into a flex-array. That would involve the
double-format you mention, but we could use it in lots of places to make
the code nicer. Maybe like:

  void *fmt_flex_array(size_t base, const char *fmt, ...)
  {
          va_list ap;
	  size_t flex;
	  unsigned char *ret;

	  va_start(ap, fmt);
	  flex = vsnprintf(NULL, 0, fmt, ap);
	  va_end(ap);

	  ret = xmalloc(base + flex + 1);
	  va_start(ap, fmt);
	  /* Eek, see below */
	  vsnprintf(ret + flex, flex + 1, fmt, ap);

	  return ret;
  }

and you'd call it like:

  struct name_decoration *r = fmt_flex_array(sizeof(*r), "dist=%d", x);

Except that I don't think we are guaranteed that offsetof(mystruct,
flex_member) is equal to sizeof(mystruct). If FLEX_ARRAY is 0, it should
be, but some platforms use FLEX_ARRAY=1. So you'd have to pass in the
offset like:

  struct name_decoration *r = fmt_flex_array(sizeof(*r),
                                             offsetof(*r, name),
					     "dist=%d", x);

which is a little less nice. You could make it nicer with a macro, but
we don't assume variadic macros. <sigh>

-Peff