Re: [PATCH] Avoid allocating 0 bytes, was Re: [PATCH 4/4] git-compat-util.h: dietlibc-friendly x{malloc,realloc,calloc}
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Dec 26, 2005, 20:34 UTC
- Message-ID
- <Pine.LNX.4.63.0512262134290.19331@wbgn013.biozentrum.uni-wuerzburg.de>
- In-Reply-To
- <7vzmmnisix.fsf@assigned-by-dhcp.cox.net>
Hi,
On Mon, 26 Dec 2005, Junio C Hamano wrote:
Show 20 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > diff --git a/csum-file.c b/csum-file.c > > index 5f9249a..2c0f097 100644 > > --- a/csum-file.c > > +++ b/csum-file.c > > @@ -121,6 +121,9 @@ int sha1write_compressed(struct sha1file > > unsigned long maxsize; > > void *out; > > > > + if (size == 0) > > + return 0; > > + > > memset(&stream, 0, sizeof(stream)); > > deflateInit(&stream, Z_DEFAULT_COMPRESSION); > > maxsize = deflateBound(&stream, size); > > I think this and the one in sha1_file.c::write_sha1_file() are > wrong; 0-size input would not result in 0-size output. Have you > tested them by actually exercising the codepaths you touched?
No, I did not test them. At least not consciously (I did the whole work on Dec 24, when I was lying in bed, ill, trying to distract myself from being miserable).
The reason I did it this way: If zlib inflates a buffer of 0 bytes, it makes no sense to expect anything than 0 bytes to come out of it, right? Therefore, if zlib encounters a deflated buffer of 0 bytes, it should inflate it to 0 bytes. So it is a good idea in any case to write out 0 bytes.
However, thinking of it again, it might break backwards compatibility. But I don't think so.
Show 8 quoted lines
> > diff --git a/diffcore-pathspec.c b/diffcore-pathspec.c > > index 68fe009..a12337a 100644 > > --- a/diffcore-pathspec.c > > +++ b/diffcore-pathspec.c > > @@ -48,10 +48,14 @@ void diffcore_pathspec(const char **path > > diffcore-pathspec and diffcore-order can probably return without > touching diff_queued_diff if there is no work to be done.
To be honest: I did not completely understand what that code does. It seemed obvious, but I got a track record of breaking things. So I was cautious. I will not try to modify it myself without a good test case.
Show 14 quoted lines
> > @@ -353,7 +353,8 @@ static void write_index_file(const char
> > {
> > struct sha1file *f;
> > struct object_entry **sorted_by_sha =
> > - xcalloc(nr_objects, sizeof(struct object_entry *));
> > + xcalloc(nr_objects ? nr_objects : 1,
> > + sizeof(struct object_entry *));
> > struct object_entry **list = sorted_by_sha;
> > struct object_entry **last = sorted_by_sha + nr_objects;
> > unsigned int array[256];
>
> This can be simplified by sorted_by_sha = list = last = NULL
> when nr_objects == 0 and avoiding qsort; that is what you did in
> pack-objects, I think.If nr_objects == 0, we could write out a fixed index.
We could alternatively exit(1), because what sense does it make to have an index if the pack contains 0 objects? However, this has consequences: Every user of index-pack (does "fetch-pack --keep" call it?) has to be aware that empty packs are illegal.
Again, I did this so that the tool does not break. I'll try to come up with something better soon.
Show 16 quoted lines
> > @@ -448,8 +449,10 @@ int main(int argc, char **argv) > > > > open_pack_file(); > > parse_pack_header(); > > - objects = xcalloc(nr_objects, sizeof(struct object_entry)); > > - deltas = xcalloc(nr_objects, sizeof(struct delta_entry)); > > + objects = xcalloc(nr_objects ? nr_objects : 1, > > + sizeof(struct object_entry)); > > + deltas = xcalloc(nr_objects ? nr_objects : 1, > > + sizeof(struct delta_entry)); > > parse_pack_objects(); > > free(deltas); > > write_index_file(index_name, sha1); > > Likewise I suspect. After all the special case is only when > reindexing an empty pack ;-).
Right.
Show 6 quoted lines
> My inclination is to do things in these steps: > > - apply cleanups that actually simplify the logic, while > leaving the ones that you needed to do (size ? size : 1) > unmodified (BTW, must next_in/next_out point at non NULL when > avail_in/avail_out are zero?).
I was cautious. Maybe at some point in future, zlib relies on that. But it is probably better to try not calling zlib at all in these cases.
Show 13 quoted lines
> - change x*alloc like this, once the above is done:
>
> static inline void *xmalloc(size_t size)
> {
> void *ret = malloc(size);
> #ifdef MALLOC_CAN_RETURN_NULL_ON_0SIZE
> if (!ret && !size)
> ret = malloc(size+1);
> #endif
> if (!ret)
> die("Out of memory, malloc failed");
> return ret;
> }Sounds easiest.
Ciao, Dscho