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

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 [off-list ref] wrote:

quoted

quoted

      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