Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/8546 )
Change subject: IMPALA-5624: Replace "ls -l" with opendir() in ProcessStateInfo ...................................................................... Patch Set 3: (3 comments) > Based on my reading of "man readdir", it's not threadsafe. I think > the only usage here is if a user hits "http://impalad:.../"; to see > the web UI. It's possible that there's a lock somewhere preventing > concurrent use, but given that this is already reasonably > expensive, I'd recommend using the reentrant readdir_r instead. > > I looked around, and it looks like C++17 and boost have filesystem > abstractions, but I think readdir() is simple enough. I have replaced readdir() with readdir_r(), but the solution is only ok for Linux, and may cause problems on other Unix systems, because the size of dirent.d_name is not always fix. The general solution in linux.die.net/man/3/readdir_r can be actually problematic, see womble.decadent.org.uk/readdir_r-advisory.html The scenario is not. It is easy to allocate a buffer whose size will be surely enough in /proc/self/fd, but I am unsure about the ideal solution. - Should I care about portability to other Posix variants? - Is there a way to express "compilation error if other Posix variant than Linux"? - Is there a maximum size for filenames? Most examples use 255 (+1 for \0), but I do not know if it is an official value or not. - Is there a guideline for the maximum size to allocate on stack? Is ~255 ok? http://gerrit.cloudera.org:8080/#/c/8546/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8546/2//COMMIT_MSG@7 PS2, Line 7: IMPALA-5624: Replace "ls -l" with opendir() in ProcessStateInfo > Please describe in this line what the change does, not what it should do, i Done http://gerrit.cloudera.org:8080/#/c/8546/2//COMMIT_MSG@11 PS2, Line 11: Posix > nit: Posix is a name and should be capitalized. Done http://gerrit.cloudera.org:8080/#/c/8546/2//COMMIT_MSG@18 PS2, Line 18: way to know the "expected value" in advance, and the number of file desciptors can > Couldn't we test that a reasonable minimum number of file descriptors is re Done -- To view, visit http://gerrit.cloudera.org:8080/8546 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibffae8069a62e100abbfa7d558b49040b095ddc0 Gerrit-Change-Number: 8546 Gerrit-PatchSet: 3 Gerrit-Owner: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Comment-Date: Thu, 16 Nov 2017 01:46:42 +0000 Gerrit-HasComments: Yes
