Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
interfaces/mount: add partial implementation of Change.Needed #3209
Conversation
All checks have passed
| + // | ||
| + // NOTE: This disregards mount options and and any optional fields that | ||
| + // indicate sharing. Snapd doesn't use those properties yet, except for | ||
| + // read-only bind mounts but those are special anyway. |
niemeyer
Apr 25, 2017
Contributor
Why is it ignored, though? I mean, why would we support flags, yet ignore them? Sounds like Needed should report whether it's needed based on whether the information changed.
zyga
Apr 27, 2017
Contributor
On second thought I think we do need to look at flags. This is also made a little bit more complicated because there are two sets of flags (superblock and mount) and they mean different things.
| + switch c.Action { | ||
| + case Mount: | ||
| + for _, mie := range mounted { | ||
| + if fe.Dir == mie.MountDir && fe.Type == mie.FsType && fe.Name == mie.MountSource && mie.Root == "/" { |
zyga
Apr 27, 2017
Contributor
Root is the part of the mounted device that is really mounted at MountDir. Since this branch is not trying to support bind mounts that part is always "/" for normal mounts.
niemeyer
Apr 27, 2017
Contributor
I still don't understand why this is relevant. This logic shouldn't really care whether it's a bind mount or not. If it changed, it's not the same anymore, and it ought to say so.
| + return true | ||
| + case Unmount: | ||
| + for _, mie := range mounted { | ||
| + if fe.Dir == mie.MountDir && fe.Type == mie.FsType && fe.Name == mie.MountSource && mie.Root == "/" { |
niemeyer
Apr 25, 2017
Contributor
These are both exactly the same algorithm, so we can pull it out of the switch and perform the comparison ahead of time, and then inside the switch simply take the decision of whether to return true/false or not.
zyga
Apr 27, 2017
Contributor
I think you are right now but as the bind mount logic is added those won't look the same (I think). If it is still the same later we can optimize this part.
niemeyer
Apr 27, 2017
•
Edited 1 time
-
niemeyer
Apr 27, 2017
Contributor
-
Apr 27, 2017
The best practice is actually the opposite: do it tersely while you can, and then expand as necessary. Having that sort of trivially duplicated logic anticipating potential needs makes it harder to have better code later, because your mind is always looking at more logic than it ought to.
|
This PR needs some love still. |
|
I'm closing this as I think we don't really need it anymore. |
zyga commentedApr 19, 2017
This patch adds a partial implementation of Change.Needed that works for
things that aren't bind mounted. Bind mounting requires some additional
detection because the fstab entry doesn't contain the whole information
and that needs to be reconstructed by looking at mountinfo.
To be precise, the source of the bind mount is only defined indirectly.
In a simple bind mount /src -> /dst the /src is a "pointer" to whatever
is mounted there (cound be something mounted directly at /src or could
be a fragment of the bigger mount at /).
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com