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. 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… :/ > 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). 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. Best regards David Kalnischkies
Attachment:
signature.asc
Description: Digital signature