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 function for saving fstab-like file #3114
Conversation
zyga
added some commits
Mar 30, 2017
Some checks were not successful
|
LGTM |
chipaca
reviewed
Mar 31, 2017
The changes themselves seem alright, but and I appreciate that taking an io.Writer is the most general, but I worry that using this together with osutil.AtomicWriteFile is harder than it needs to be, meaning we won't write out fstabs atomically.
If the expectation is that this would be used to write things to stdout or logfiles for debug purposes, then it's fine.
|
@chipaca interesting insight, we could have an atomic helper or just write to a string and write the string. |
All checks have failed
Some checks were not successful
| +// LoadFSTab reads and parses an fstab-like file. | ||
| +// | ||
| +// The supported format is described by fstab(5). | ||
| +func LoadFSTab(reader io.Reader) ([]Entry, error) { |
mvo5
Apr 3, 2017
Collaborator
(nitpick^2) - how about LoadFstab/SaveFstab ? Slightly easier to type and /etc/fstab is also fstab instead of FSTab?
zyga
Apr 3, 2017
Contributor
I was trying to follow the acronym (file system table) but I have no strong opinions about it. We can rename this to {Load,Save}Fstab if you want.
| +// Note that there is no attempt to use atomic file write/rename tricks. The | ||
| +// created file will typically live in /run/snapd/ns/$SNAP_NAME.fstab and will | ||
| +// be done so, while holidng a flock-based-lock, by the snap-update-ns program. | ||
| +func SaveFSTab(writer io.Writer, entries []Entry) error { |
mvo5
Apr 3, 2017
Collaborator
Not sure if it matters, but might be worth documenting in the docstrings. The fact that we strip all comments from the fstab we load earlier means when we save it again some information is potentially lost.
mvo5
reviewed
Apr 3, 2017
Some smallish comments for your consideration. Looks good just my usual nitpick :)
| +func SaveFSTab(writer io.Writer, entries []Entry) error { | ||
| + var buf bytes.Buffer | ||
| + for i := range entries { | ||
| + if _, err := buf.WriteString(entries[i].String()); err != nil { |
mvo5
Apr 3, 2017
Collaborator
This could be written as if _, err := fmt.Fpritnf(buf, "%s\n", entries[i]); err != nil { unless I'm mistaken, i.e. you can avoid the extra 2 lines for the buf.WriteRune() below.
zyga
added some commits
Apr 3, 2017
Some checks were not successful
| - return err | ||
| - } | ||
| - if _, err := buf.WriteRune('\n'); err != nil { | ||
| + if _, err := fmt.Fprintf(&buf, "%s\n", entries[i].String()); err != nil { |
mvo5
Apr 3, 2017
Collaborator
The .String() in entries[i] is not needed, the %s will run this implicitly.
zyga
added some commits
Apr 3, 2017
All checks have failed
Some checks were not successful
| +// Note that there is no support for comments, both the LoadFSTab function and | ||
| +// SaveFSTab just ignore them. | ||
| +// | ||
| +// Note that there is no attempt to use atomic file write/rename tricks. The |
niemeyer
Apr 3, 2017
Contributor
Uh oh.. why is this not atomic? It's not just concurrency.. the system can power off at any moment.
niemeyer
Apr 3, 2017
•
Edited 1 time
-
niemeyer
Apr 3, 2017
Contributor
-
Apr 3, 2017
First, I don't think this is true. These files also go into /var/lib/snapd/mount, right?
Second, even if this is true, a snapd abrupt termination would leave the system in a bad state.
Then, how come this was merged without the discussion getting to a conclusion?
chipaca
Apr 3, 2017
Member
Enhance your calm:
C: zyga— what's the expected use case for
SaveFSTab?
Z: Chipaca: snap-update-ns will write the things it did to /run
Z: Chipaca: it will be a file in /run/snapd/ns/$SNAP_NAME.fstab
C: so, /run doesn't need atomicity in the sense we mean there
Z: Chipaca: it should match /var/lib/snapd/mount/
C: but /var does
Z: Chipaca: but if it crashes the code will pick up from the start
Z: Chipaca: and see if it needs to mount anything
C: ah, if snapd crashes? yeah
Z: Chipaca: not snapd, this will be in a separate process
C: k
C: i was thinking of the machine crashing, not the program crashing
Z: Chipaca: if machine crashes it's all good
Z: Chipaca: this is purely ephemeral
C: yes
C: zyga— in what sense should things in run match things in var?
Z: if all is good yes
Z: some changes can be rejected
Z: so they will not be mounted
Z: and such entries won't show up
Z: some entries can fail for various reasons
Z: this is just a helper for the algorithm we merged earlier
Z: that descries 'current' state
Z: the desired state is always in /var/
Z: written by snapd
Z: using all the nice atomic helpers
Z: and the current file will be only written by this thing
Z: and maybe by snap-confine but this is really optional since it will cross-check with mountinfo to see what is really mounted
C: zyga— ok. Can you add a note about this to the method?
Z: Chipaca: sure
C: zyga, thanks
niemeyer
Apr 4, 2017
Contributor
@chipaca Well, exactly. /var/lib/snapd/mount is full of .fstab files. I do hope we use those same helpers to write content there instead of having two different ways to write those files. So it's not ephemeral, and should definitely be written atomically.
Despite that, I am calm. We just need to make sure we're not fine with corrupt state lying around.
cc @zyga
zyga
Apr 4, 2017
Contributor
Yeah, those discussions made me rethink this a little. Please have a look at #3135
zyga commentedMar 30, 2017
This is stacked on top of #3103
There's one new function, nothing fancy as it reuses all the work from earlier branches.