From reviews-return-50402-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Fri Nov 18 01:24:49 2016 Return-Path: X-Original-To: apmail-mesos-reviews-archive@minotaur.apache.org Delivered-To: apmail-mesos-reviews-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id AB5B6199CE for ; Fri, 18 Nov 2016 01:24:49 +0000 (UTC) Received: (qmail 76113 invoked by uid 500); 18 Nov 2016 01:24:49 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 76081 invoked by uid 500); 18 Nov 2016 01:24:49 -0000 Mailing-List: contact reviews-help@mesos.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: reviews@mesos.apache.org Delivered-To: mailing list reviews@mesos.apache.org Received: (qmail 76060 invoked by uid 99); 18 Nov 2016 01:24:49 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 18 Nov 2016 01:24:49 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 164E42D8CC6; Fri, 18 Nov 2016 01:24:47 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============1314443804496906129==" MIME-Version: 1.0 Subject: Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files. From: Jiang Yan Xu To: Jie Yu , Kevin Klues , Jiang Yan Xu , Gilbert Song Cc: James Peach , mesos Date: Fri, 18 Nov 2016 01:24:47 -0000 Message-ID: <20161118012447.20484.64104@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: Jiang Yan Xu X-ReviewGroup: mesos X-Auto-Response-Suppress: DR, RN, OOF, AutoReply X-ReviewRequest-URL: https://reviews.apache.org/r/53791/ X-Sender: Jiang Yan Xu References: <20161115233447.20485.61538@reviews.apache.org> In-Reply-To: <20161115233447.20485.61538@reviews.apache.org> X-ReviewBoard-Diff-For: src/tests/containerizer/rootfs.cpp Reply-To: Jiang Yan Xu X-ReviewRequest-Repository: mesos --===============1314443804496906129== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53791/#review156259 ----------------------------------------------------------- src/tests/containerizer/rootfs.cpp (line 58) This feels like an implementation of `ldd()` that coule be generally useful. Would it make sense to rename ldcache.hpp|cpp as ld.hpp|cpp and add `ldd()` as a method in ld.hpp|cpp make sense? src/tests/containerizer/rootfs.cpp (lines 58 - 61) This feels like an intermediate method for recursion. Would be nice to put a front-end method above it: ``` Try> ldd(const string& path); ``` src/tests/containerizer/rootfs.cpp (line 61) Using `hashset` would make it more explicit that no order is implied (otherwise this has to be a vector). And we can use `contains()`! src/tests/containerizer/rootfs.cpp (lines 69 - 71) It feels like it's not worth extracting out the logic (similar code from volume.cpp) ``` Try load = elf::File::load(realpath.get()); if (load.isError()) { return Error("Failed to elf::File::load '" + realpath.get() + "':" " " + load.error()); } Owned file(load.get()); ``` into a 10-line helper method and spend 4 lines here? src/tests/containerizer/rootfs.cpp (line 73) Turn it into a hashset for easier checking? src/tests/containerizer/rootfs.cpp (lines 81 - 84) If we have to do this, format it this way ``` auto entry = std::find_if( cache.begin(), cache.end(), [dependency](const ldcache::Entry& e) { return strings::startsWith(e.name, dependency); }); ``` But is prefix matching necessary? Can't we do extract matching (i.e.,`hashset.contains()`)? Note that our seed list are just programs without versions. src/tests/containerizer/rootfs.cpp (line 82) No space. src/tests/containerizer/rootfs.cpp (line 107) Not convinced that we need to extract it out since it's used only in a single place? src/tests/containerizer/rootfs.cpp (line 196) The following is more widely used. ``` #ifdef __linux__ ``` src/tests/containerizer/rootfs.cpp (lines 196 - 201) The whole file should just be linux only so we don't have to do it here. src/tests/containerizer/rootfs.cpp (line 223) Would the following look better? ``` hashset needed(programs.begin(), programs.end()); foreach (const string& program, programs) { Try> dependencies = ldd(file); // error handling. needed = needed | dependencies; } ``` - Jiang Yan Xu On Nov. 15, 2016, 3:34 p.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53791/ > ----------------------------------------------------------- > > (Updated Nov. 15, 2016, 3:34 p.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu. > > > Bugs: MESOS-6588 > https://issues.apache.org/jira/browse/MESOS-6588 > > > Repository: mesos > > > Description > ------- > > The dependencies for the programs we need in the Linux root > filesystem vary over time and across distributions. Since Stout > has support for parsing the library dependencies of ELF binaries, > use this to collect the necessary dependencies when constructing > a root filesystem for the tests. > > > Diffs > ----- > > src/tests/containerizer/rootfs.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/53791/diff/ > > > Testing > ------- > > sudo make check (Fedora 24) > > > Thanks, > > James Peach > > --===============1314443804496906129==--