[PATCH 1/2] Added use of xmalloc() on diff-delta.c

[PATCH 1/2] Added use of xmalloc() on diff-delta.c

[Bruno Ribas]

Signed-off-by: Bruno Ribas <ribas@c3sl.ufpr.br>
---
 diff-delta.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/diff-delta.c b/diff-delta.c
index 9f998d0..74a8377 100644
--- a/diff-delta.c
+++ b/diff-delta.c
@@ -157,9 +157,8 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)
 	memsize = sizeof(*index) +
 		  sizeof(*hash) * hsize +
 		  sizeof(*entry) * entries;
-	mem = malloc(memsize);
-	if (!mem)
-		return NULL;
+	mem = xmalloc(memsize);
+
 	index = mem;
 	mem = index + 1;
 	hash = mem;
@@ -258,9 +257,7 @@ create_delta(const struct delta_index *index,
 	outsize = 8192;
 	if (max_size && outsize >= max_size)
 		outsize = max_size + MAX_OP_SIZE + 1;
-	out = malloc(outsize);
-	if (!out)
-		return NULL;
+	out = xmalloc(outsize);
 
 	/* store reference buffer size */
 	i = index->src_size;
-- 
1.5.0.3

[PATCH 2/2] Removed NULL check on builtin-pack-objects.c from create_delta_index() as it just checks for Out of Memory

[Bruno Ribas]

Signed-off-by: Bruno Ribas <ribas@c3sl.ufpr.br>
---
 builtin-pack-objects.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index b5f9648..04a4abc 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1254,8 +1254,6 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
 	}
 	if (!src->index) {
 		src->index = create_delta_index(src->data, src_size);
-		if (!src->index)
-			die("out of memory");
 	}
 
 	delta_buf = create_delta(src->index, trg->data, trg_size, &delta_size, max_size);
-- 
1.5.0.3

Re: [PATCH 1/2] Added use of xmalloc() on diff-delta.c

[Junio C Hamano]

These two functions, create_delta_index() and create_delta(),
are already nicely libified.  They allow the caller to deal with
oom condition.  The caller may die(), or it may decide to
continue its operation with reduced functionality without using
delta data.  A good example of this is found a few lines after
the lines the second patch touches.  When create_delta() cannot
find memory to work with, the entire function returns 0, saying
"sorry, cannot deltify these two", which would cause the object
stored without deltification.

These patches take that nice property away, making libification
more difficult, which is the downside.  Is there an upside?

If anything, I suspect that the part that calls die() you
touched in the second patch could return NULL.

Re: [PATCH 1/2] Added use of xmalloc() on diff-delta.c

[Linus Torvalds]

On Wed, 4 Apr 2007, Junio C Hamano wrote:
> 
> These patches take that nice property away, making libification
> more difficult, which is the downside.  Is there an upside?

Well, we could just make the libification rule very simple:

 - the library does *not* include "xmalloc()", and you have to handle 
   out-of-memory situations yourself inside the xmalloc() that *you* as a 
   libification user provide!.

Then, we just make our xmalloc() be non-inlined (which we should do 
*anyway* - it's long since grown so big that it shouldn't be inlined in 
the first place), and we make it part of a non-library git object file.

Other libgit uses might end up doing something like

		..
		if (sigsetjump(buffer, 1)) {
			show_oom_message();
		..

	void *xmalloc(size_t size)
	{
		void *ret = malloc(size ? size : 1);
		if (!ret)
			siglongjmp(buffer);
		return ret;
	}

or, if they use C++ exception handling, they'd just make their own 
xmalloc() raise an exception, and have the callers catch it.

The point being that this is what you'd need to do *anyway*, and trying to 
make all the library routines return NULL or some other error case is just 
worse programming practice than just having a xmalloc() that dies by 
default but that can be overridden.

		Linus

Re: [PATCH 1/2] Added use of xmalloc() on diff-delta.c

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Wed, 4 Apr 2007, Junio C Hamano wrote:
>> 
>> These patches take that nice property away, making libification
>> more difficult, which is the downside.  Is there an upside?
>
> Well, we could just make the libification rule very simple:
>
>  - the library does *not* include "xmalloc()", and you have to handle 
>    out-of-memory situations yourself inside the xmalloc() that *you* as a 
>    libification user provide!.
>
> Then, we just make our xmalloc() be non-inlined (which we should do 
> *anyway* - it's long since grown so big that it shouldn't be inlined in 
> the first place), and we make it part of a non-library git object file.

I agree that is probably a sane thing to do for existing callers
of xmalloc() -- the callers are not prepared to handle
(near-)oom case gracefully to begin with.

Your "libified git decides how xmalloc() copes with (near-)oom
condition gracefully" is much better than "memory-allocating
function in git declares that oom in xmalloc() is fatal", which
is what we currently have.

I however think it is an independent issue, wrt the part Bruno's
patch touches.  In the case of this particular call chain
between create_delta()/create_delta_index() and its caller, I
think the code that is there allows nicer arrangement.  The
caller could instead easily attempt to cope with (near-)oom
condition more gracefully.  And that was my suggestion about
returning 0 when delta_index cannot be built instead of dying.

Of course, this caller is in memory-hungry "pack-objects", and
all of the above is mostly academic, as failure to allocate
memory in create_delta_index() would most likely mean you would
have trouble allocating memory for other more important data.