Mark Wielaard <[email protected]> writes: > splice_cat can leave open file descriptors around. This breaks the > valgrind testsuite with --track-fds=yes. This is fairly innocent, like > still reachable memory, but does need a new suppression when tracking > file descriptor handling. Introduce a new splice_cat_close function > that can be used on error from splice_cat and before exiting main. > > * src/cat.c (splice_cat): Move static pipefd and pipefd_pipe_size to > file level, guarded by HAVE_SPLICE. Move pipefd close and cleanup from > error handler to... > (splice_cat_close): New function. Empty if we don't HAVE_SPLICE. > (main): Call splice_cat_close before exit. > --- > src/cat.c | 47 +++++++++++++++++++++++++++++++++++------------ > 1 file changed, 35 insertions(+), 12 deletions(-)
Hey Mark, thanks for the patch! I forgot to respond to you on the Valgrind mailing list [1], but I'll just do so here since it mostly relates to coreutils anyways. You said: > Although it would be great if you could just clean up and close these > descriptors, we probably should just write our own little cat like > program. We just picked cat since it seemed all implementations always > closed all file descriptors it opened. But coreutils cat is more > complicated than that. Yes. I would have suggested using another program, but there are some cases where using splice might be useful for them, too. E.g., 'head -c' could probably benefit. Whether that is worth it or not is another question. Anyways, I'm fine with closing the file descriptors, since leaving them open and not checking if close fails is generally bad practice. This specific case of the pipe being empty is niche enough that we probably should just close them to save people time investigating why the leak occurs. > That said you might want to run valgrind --track-fds=yes (or maybe > just --track-fds=bad which only reports on bad file descriptor usage, > not "leaking" fds) in the coreutils testsuite. It really is pretty > useful for catching possible file descriptor misuse. I agree it would be helpful. We have a script to run tests using Valgrind, but it is a bit of manual work to use [2]. As tends to be the case when testing things is not automatic, that means I have not run it recently. It would be good to improve that so it could be run automatically, e.g., with a configure or make option. However, I suspect it will be a bit tricky. We use both 'strace' and LD_PRELOAD to test some behaviors that you typically wouldn't run into, or that are tricky to reproduce consistently. I'm not too sure if/how Valgrind will interact with those. Thanks again, Collin [1] https://sourceforge.net/p/valgrind/mailman/message/59419557/ [2] https://github.com/coreutils/coreutils/blob/9cad3af5ce6301b2fadad006c84f39e383bc8e46/README-valgrind
