On Thu, Mar 20, 2014 at 01:54:22PM +0100, david@kalnischkies.de wrote: > Hi Julian, > > On Mon, Mar 17, 2014 at 02:01:47PM +0100, Julian Andres Klode wrote: > > We dup() the file descriptor when opening compressed files, so we > > always need to close the dup()ed one. Furthermore, not unsetting > > Do we? Line 1197 (with all your patches applied) is the only one dup'ing > the file descriptor and only if it isn't allowed to autoclose and using > a library to open the file (as the library functions autoclose) > [library using FileFd's have the Compressed flag set]. > And CloseDown does not deal with closing fds at all. It closes via > library calls and reaps zombies from compressor forks. Yes, the description needs to be tweaked, it's wrong, you're right. > > In fact, I smell missing closes here for e.g. d->compressed_fd which > stores the given fd if we use a compressor pipe and OpenDescriptor > (in other words: unused code at the moment as we have a library for > all compressors now). This code really has a billion edge cases and > for very few we have tests covering them… :/ It might make sense to split all the various backends gzip / bzip2 / xz / raw file out into separate (internal) backend classes and abstract that using classes with virtual methods. This should reduce the number edge cases and combinations, and FileFd itself can focus on the real features. > > > the d-pointer causes issues when running OpenDescriptor() multiple > > times on the same file descriptor. > > I am not sure why I did it this way, but I guess the reason is gone. > At least I can't imagine a reason for it now and would say: do it. > > The rest looks fine, even through I am still puzzled everytime I see the > usage of ',' outside of for-loop-headers and variable definitions (2nd). I'll fix this. I forgot to change this before posting the patches. > > Regarding the 3rd: gcc reports "unused parameter Force" and this is > actually a good one: FileFd ignores the returncode of the binaries it > has executed (if it did at all), which feels wrong, but on the other > hand the close methods of the libraries are usually void, so no error > either… which is why I dropped this here rather than below the other > patches, I guess we have to improve the close handling a bit further > than this, even if it is a good start. As it is patch 3 would be a > behaviour change otherwise, even through quiet subtil. So should there be a new FileFd::Close(bool Force) instead? I'm not sure ignoring the error in FileFd::Close() is a good idea, if we have the information, we should use it. The only place we cannot get any information is if bzip2 fails, but I don't know if that's possible, most certainly not on reading (and ExtractTar only reads). We currently have another bug: If lzma destructor fails, it sets an error, but we then do not return false in InternalClose(). Instead of returning true at the end, we should probably simply return !_error->PendingError(); this way we catch any error we set somewhere. So, I'll fix the description of patch 1 and drop the comma operator in patch 2 and push those two to debian/sid. We can then think how to proceed with patch 3 and the Force thing. -- Julian Andres Klode - Debian Developer, Ubuntu Member See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/. Please do not top-post if possible.
Attachment:
pgprBpjZihdL9.pgp
Description: PGP signature