tar bugfix patch n.347 (recoded, version 4)

Thu Dec 22 12:05:10 UTC 2005

Rob Landley ha scritto:
> 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.

  I will do both.

> 
> +       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.
> 

  I return EXIT_FAILURE because I see some applets need FAIL return code 
to delete tmp-file which are not inflate correctly, in particular when 
diskspace is not enought to complete writing. So is what I understood of 
use of this code. If I am wrong I spent only a return too much before 
end. I think optimization are important in loop, not so much in exiting.

> 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?)
> 

  I removed the memory leak comment because I think you are right but I 
left frees before returning because I do not think they hurt or slowdown 
too much because:

  if bb is not exiting, free are required
  if bb is exiting so delaying it by two free does not hurt too much

  In the case frees make segmentation fault that probably hurts much 
more in non-exiting case. Memory leaks in my opinion could happen when a 
bb generates zombies even in this case are not *really* memory leaks.
  Zombies could happen and they waste the system memory, if I make frees 
before exit I should waste much less memory because zombies are as 
little as possible.

  About vfork/fork and _exit/exit the only think I can say for sure is 
my great ignorance. So actually the only things I know are from here:

  http://www.erlenstar.demon.co.uk/unix/faq_2.html

  which document are talking about zombies too and how to preventing 
they happen. Solaris docs was read just because I worked on solaris hw 
but actually I do not have any such hw.

> 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?)
> 

  You are right in that patch have not appear any makefile changes.
  Anyway I changed makefile because diff produce an output in which:

  - *.c~ was found
  - docs/busybox.net empty dir was found

  Please note that rmdir without any option do not remove directory 
containing some files/subdirs but only empty dir (at least on my 
workstation, alias may happen). So I do not think it could hurt but 
simple remove diff issue.

> 
>>./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...?)
> 

  Because the second messagge happen in the parent process and cause it 
to die with exit code 1, otherwise it would exit with 0 even if child 
failed. The first message happen in the child, but open_transformer did 
not have the capability to catch the child's exit code. The patch 
attached in this mail make open_transformer able to catch child exit 
code. I read that code should prevent zombies happen too.

  Even the open_transformer has changed in this way, I think that zero 
lenght msg are good because it happen in *that* proccess, or happen for 
zero lenght file/data or not happen at all if the child management was 
done. So I do not removed that message.

  These outputs are related to the patch attached:

[roberto at wsraf busybox-1.01_raf]$ ./busybox tar tvzf /tmp/test.tar.gz
-rw-r--r-- 0/0    133490 2005-11-18 10:20:23 ../volantino_LD05.pdf

[roberto at wsraf busybox-1.01_raf]$ ./busybox tar tvzf /tmp/test.tar.gz0
tar: unexpected end of file		<-- happen in the child
tar: child exit with error code 1	<-- reported in the parent

  The second message could be removed but I do not like a parent which 
die for an error without saying anything because nobody could tell me if 
that child had a verbose or silent death.

  [cut]

>>  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?
> 

  Sorry I do not understand this, probably my english is not good 
enought. If you was saying you would like to see another patch...
  ...look at the tail of this msg.
   :-)

  Cheers,
-- 
Roberto A. Foglietta
http://www.roberto.foglietta.name
SkypeNick: robang74
ICQ#: 108718257
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox-1.01_raf_4.diff
Type: text/x-patch
Size: 6746 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20051222/7172a07e/attachment.bin 