[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: tar bugfix patch n.347 (recoded, AGAIN)
From:       Rob Landley <rob () landley ! net>
Date:       2005-12-21 22:21:27
Message-ID: 200512211621.27671.rob () landley ! net
[Download RAW message or body]

On Wednesday 21 December 2005 14:59, Roberto A. Foglietta wrote:
> Hi,
>
>      patch update in order to have it in 1.1.0
>      http://bugs.busybox.net/view.php?id=347

It's easier on me, at least, if you attach your patch to the email rather than 
to the bug.

+       if (nwrote == -1) {
+               bb_perror_msg("unzip write failed");
+               return EXIT_FAILURE; //any return or die will leak memory in 
bb as static link applets
+       }

We have a bb_perror_msg_and_die() that never returns, and is the same size as 
bb_perror_msg() so you save the return.

Care to explain the comment?  Right now, we depend on exit() to clean up our 
memory most of the time.  The FEATURE_CLEAN_UP stuff is a debugging aid to 
find memory leaks in loops that might blow up into something significant with 
big data sets.  (We might later add infrastructure to allow busybox to not 
depend on a kernel with a working fork/exit, but that's 1.2 or later work and 
would require adding rather a lot of infrastructure that might best be done 
as a separate "fakefork.so" library (that wraps the fork() and exec() 
families, malloc(), free(), mmap(), mumap(), open(), close(), and probably a 
whole lot more...  Or perhaps fakefork belongs in uClibc?  Who knows?)

What context is this returning to, by the way?  EXIT_FAILURE implies this is a 
return from main (since that's what that value is for).  In which case, the 
later bb_error_msg() followed by returns should also be 
bb_perror_msg_and_die()...

Don't special case gunzip like this, since we also have bzip2 support in there 
too (or will eventually)...

Your bit about it not being a good idea to use exit(): wrong.  We use exit().  
In fact, our nebulous future plans to possibly make FEATURE_CLEAN_UP more 
than a mere debug option would be totally screwed up by explicit use of 
_exit() anywhere in the code.  We'd have to find it and remove it.

Why are you referencing a SunOS 5 man page?  Do you want to port busybox to 
Sun?  (Does sun only work if you use vfork() instead of fork?)

Your makefile changes are unrelated this this patch, and the "rmdir 
docs/busybox.net" is actually _dangerous_ because the busybox.net website is 
copied from the version in svn once an hour.  So the version in the tarball 
is in fact supposed to be there, in svn at least.  (We can remove it from 
actual releases but why?)

> ./busybox tar tvzf /tmp/test.tar.gz0; echo $?
> tar: unexpected end of file
> 0
>
> AFTER patch
>
> ./busybox tar tvzf /tmp/test.tar.gz0; echo $?
> tar: unexpected end of file
> tar: null size tar archive
> 1

It was tested!  That's what I like to see. :)

Looking in the code, the redundant error message is a special case you put in, 
right before an already existing return(EXIT_FAILURE).  If you've already 
gotten an "unexpected end of file" message, why print another?  (Is that 
first message _only_ printed for compressed archives, and you need the zero 
length one for uncompressed tarballs...?)

> Supposed to fix a possible memory leak in decompress_unzip.c
> which should affect static linked one in the case of error.

Yeah, it starts with moving a return after a pair of free() calls.  What this 
has to do with static linking is anybody's guess though, since whether we're 
statically linked or not, we mostly rely on exit() to free our memory for us 
to avoid the space taken up by explicit calls to free()...

>   Sorry but I did not expect coding for bb would be so hard!
>   If somebody would be my tutor and check my patch for the first times I
> will appreciate very much.

You got some comments.  Shows promise, but not ready yet.  Want to take a 
second stab at it?

>   Cheers

Rob
-- 
Steve Ballmer: Innovation!  Inigo Montoya: You keep using that word.
I do not think it means what you think it means.
_______________________________________________
busybox mailing list
busybox@busybox.net
http://busybox.net/cgi-bin/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]