Re: Review Request 52596: Added special case when sorting hierarchically in MountInfoTable::read.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52596/ --- (Updated Oct. 12, 2016, 11:09 p.m.) Review request for mesos and Jie Yu. Changes --- It turns out that I can't preemptively keep a self reference out of the child list, otherwise the algorithm for adding entries back into the MountInfoTable vector doesn't work as expected. Instead, I add the self reference into the child list, but don't recurse down into it once it is visited. Bugs: MESOS-6118 https://issues.apache.org/jira/browse/MESOS-6118 Repository: mesos Description --- It is legal to have entries in a `MountInfoTable` whose `entry.id` is the same as `entry.parent`. This can happen (for example), if a system boots from the network and then keeps the original `/` in RAM. However, to avoid cycles when walking the mount hierarchy, we should not treat these entries as children of their parent so we skip them. This commit adds functionality to handle this case. Diffs (updated) - src/linux/fs.cpp 4b10141a49dfb3c6defdb07e295eb14cfcdd36ce Diff: https://reviews.apache.org/r/52596/diff/ Testing --- GTEST_FILTER="" make -j40 check GTEST_FILTER="FsTest.MountInfoTableReadSorted" src/mesos-tests Thanks, Kevin Klues
Re: Review Request 52596: Added special case when sorting hierarchically in MountInfoTable::read.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52596/ --- (Updated Oct. 10, 2016, 8:50 p.m.) Review request for mesos and Jie Yu. Changes --- Added blocker on upcoming commit. Bugs: MESOS-6118 https://issues.apache.org/jira/browse/MESOS-6118 Repository: mesos Description --- It is legal to have entries in a `MountInfoTable` whose `entry.id` is the same as `entry.parent`. This can happen (for example), if a system boots from the network and then keeps the original `/` in RAM. However, to avoid cycles when walking the mount hierarchy, we should not treat these entries as children of their parent so we skip them. This commit adds functionality to handle this case. Diffs (updated) - src/linux/fs.cpp 4b10141a49dfb3c6defdb07e295eb14cfcdd36ce Diff: https://reviews.apache.org/r/52596/diff/ Testing --- GTEST_FILTER="" make -j40 check GTEST_FILTER="FsTest.MountInfoTableReadSorted" src/mesos-tests Thanks, Kevin Klues
Re: Review Request 52596: Added special case when sorting hierarchically in MountInfoTable::read.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52596/#review151916 --- Patch looks great! Reviews applied: [52597, 52596] Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh - Mesos ReviewBot On Oct. 6, 2016, 7:32 a.m., Kevin Klues wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52596/ > --- > > (Updated Oct. 6, 2016, 7:32 a.m.) > > > Review request for mesos and Jie Yu. > > > Bugs: MESOS-6118 > https://issues.apache.org/jira/browse/MESOS-6118 > > > Repository: mesos > > > Description > --- > > It is legal to have entries in a `MountInfoTable` whose `entry.id` is > the same as `entry.parent`. This can happen (for example), if a system > boots from the network and then keeps the original `/` in RAM. > However, to avoid cycles when walking the mount hierarchy, we should > not treat these entries as children of their parent so we skip them. > > This commit adds functionality to handle this case. > > > Diffs > - > > src/linux/fs.cpp 4b10141a49dfb3c6defdb07e295eb14cfcdd36ce > > Diff: https://reviews.apache.org/r/52596/diff/ > > > Testing > --- > > GTEST_FILTER="" make -j40 check > GTEST_FILTER="FsTest.MountInfoTableReadSorted" src/mesos-tests > > > Thanks, > > Kevin Klues > >
Re: Review Request 52596: Added special case when sorting hierarchically in MountInfoTable::read.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52596/#review151838 --- Ship it! The patch LGTM. I would suggest we add some tests in a separate patch to increase the confidence. We could introduce a `MountInfoTable::read` that takes a string. The existing `MountInfoTable::read` can just call that overload once it gets the mount table from the system. Then, in the test, we can construct some mount table entries to simulate situations like this and verify that our code handles it properly. What do you think? - Jie Yu On Oct. 6, 2016, 7:32 a.m., Kevin Klues wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52596/ > --- > > (Updated Oct. 6, 2016, 7:32 a.m.) > > > Review request for mesos and Jie Yu. > > > Bugs: MESOS-6118 > https://issues.apache.org/jira/browse/MESOS-6118 > > > Repository: mesos > > > Description > --- > > It is legal to have entries in a `MountInfoTable` whose `entry.id` is > the same as `entry.parent`. This can happen (for example), if a system > boots from the network and then keeps the original `/` in RAM. > However, to avoid cycles when walking the mount hierarchy, we should > not treat these entries as children of their parent so we skip them. > > This commit adds functionality to handle this case. > > > Diffs > - > > src/linux/fs.cpp 4b10141a49dfb3c6defdb07e295eb14cfcdd36ce > > Diff: https://reviews.apache.org/r/52596/diff/ > > > Testing > --- > > GTEST_FILTER="" make -j40 check > GTEST_FILTER="FsTest.MountInfoTableReadSorted" src/mesos-tests > > > Thanks, > > Kevin Klues > >
