mounts: Fix bug while checking if /dev was bind-mounted #539
Conversation
|
cc @egernst |
container.go
Outdated
| c.systemMountsInfo.BindMountDev = true | ||
| } else { | ||
| c.systemMountsInfo.BindMountDev = false | ||
| if m.Destination == "/dev" { |
There was a problem hiding this comment.
Nice catch - in the original case, it would only return true of the last of the mounts happened to be the bind mount from host /dev?
Any rationale for dividing it into two if clauses?
There was a problem hiding this comment.
Also @amshinde, can we create a test to assert the expected behaviour (here, in runtime or tests)?
There was a problem hiding this comment.
@sboeuf I just add the split to make it clear we need to care of the destination first, thats all. But yeah we could very well have it as one statement.
amshinde
force-pushed
the
fix-bug-bindmount-dev
branch
from
de86549 to
bca9e98
Compare
|
LGTM
|
container.go
Outdated
| for _, m := range c.mounts { | ||
| if m.Source == "/dev" && m.Destination == "/dev" && m.Type == "bind" { | ||
| if m.Destination == "/dev" && m.Source == "/dev" && m.Type == "bind" { |
There was a problem hiding this comment.
Huh? Isn't this change is a nop (and confusing)?
There was a problem hiding this comment.
@amshinde Could you please revert that change so that we can merge that PR?
There was a problem hiding this comment.
@jodh-intel @sameo Seems like I missed repushing a change I made yesterday:(
Changes are in.
There was a bug in the way we were checking if /dev was bindmounted from the host. Signed-off-by: Archana Shinde <[email protected]>
amshinde
force-pushed
the
fix-bug-bindmount-dev
branch
from
bca9e98 to
4c2c9a4
Compare
|
lgtm
|
|
LGTM
|
|
Coveralls is "stuck" again (clearcontainers/jenkins#9), but has actually run if you look at the 16.04 CI build log: https://coveralls.io/jobs/32568132. It claims coverage fell by 3.5% but clearly it's lying, so merging. |
There was a bug in the way we were checking if /dev was
bindmounted from the host.
Signed-off-by: Archana Shinde [email protected]