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 parsing mount entries #3102
Conversation
Some checks were not successful
zyga
referenced this pull request
Mar 29, 2017
Merged
interfaces/mount: add function for parsing fstab-like file #3103
| +// ParseEntry parses a fstab-like entry. | ||
| +func ParseEntry(s string) (Entry, error) { | ||
| + var e Entry | ||
| + fields := strings.Fields(s) |
mvo5
Mar 29, 2017
Collaborator
The glibc mntent_r.c is using strsep(..., " \t") only, strings.Fields() is using any unicode whitespace. I don't think it makes a difference in practise but I would prefer if we would follow glibc as close as possible.
zyga
Mar 30, 2017
Contributor
After trying with Scanner API I now did this with the trivial strings.FieldsFunc method. Have a look please.
| + var e Entry | ||
| + fields := strings.Fields(s) | ||
| + // do all error checks before any assignments to `e' | ||
| + if len(fields) != 6 { |
mvo5
Mar 29, 2017
Collaborator
The last two fields (fs_freq and fs_passno) are optional and default to 0 if missing. So this needs to be adjusted (happens in the real world).
Also it appears that sometimes fstab entries with more fields exist in the wild, e.g. https://bugs.launchpad.net/ubuntu/+source/update-manager/+bug/873411/comments/7 - we may want to ignore those, this is similar to what glibc is doing).
zyga
Mar 29, 2017
Contributor
Aha, interesting.
The documented format of fstab doesn't have the extra entries and I don't think we should handle those. The optional aspect of the last two fields is indeed something that is easy to handle so I'll do that quickly. Thanks for noticing that!
zyga
added some commits
Mar 29, 2017
Some checks were not successful
Some checks were not successful
| + " ", `\040`, "\t", `\011`, "\n", `\012`, "\\", `\134`) | ||
| + whitespaceUnescape = strings.NewReplacer( | ||
| + `\040`, " ", `\011`, "\t", `\012`, "\n", `\134`, "\\") | ||
| +) |
chipaca
Mar 30, 2017
Member
you know you could just do
var unescape = strings.NewReplacer(...).Replaceyes?
zyga
Mar 30, 2017
•
Edited 1 time
-
zyga
Mar 30, 2017
Contributor
-
Mar 30, 2017
Oh, nice :-) I didn't think about this. Edit: applied :-)
Some checks were not successful
mvo5
merged commit 4052e82
into
snapcore:master
Mar 30, 2017
zyga
deleted the
zyga:parse-fstab-entry
branch
Mar 30, 2017
| -func escape(s string) string { | ||
| - return whitespaceReplacer.Replace(s) | ||
| -} | ||
| +var escape = strings.NewReplacer(" ", `\040`, "\t", `\011`, "\n", `\012`, "\\", `\134`).Replace |
niemeyer
Mar 31, 2017
Contributor
Instead of documenting this and then repeating it with code hoping we have both in sync, it'd be better to format the code itself in a way that may be read easily:
var escape = strings.NewReplacer(
" ", `\040`,
"\t", `\011`,
"\n", `\012`,
"\\", `\134`,
)
That sort of issue shows up very often. Sometimes that's not feasible, but when it is, code that speaks for itself is much better than comments.
| + return e, fmt.Errorf("expected between 4 and 6 fields, found %d", len(fields)) | ||
| + } | ||
| + // Parse DumpFrequency if we have at least 5 fields | ||
| + if len(fields) >= 5 { |
niemeyer
Mar 31, 2017
Contributor
In general len(fields) > N when you want to use N is easier to read than >= and N-1. So len(fields) > 5 allows you to use fields[5] for example.
| + if len(fields) >= 5 { | ||
| + df, err = strconv.Atoi(fields[4]) | ||
| + if err != nil { | ||
| + return e, fmt.Errorf("cannot parse dump frequency: %s", err) |
niemeyer
Mar 31, 2017
Contributor
In these cases, rather than printing the error it's much more useful to have the original string quoted:
return e, fmt.Errorf("cannot parse dump frequency: %q", fields[4])
zyga
Mar 31, 2017
Contributor
Aha, indeed, in fact errors from Atoi look very unreadable (ugly). I'll adjust this.
| + if len(fields) >= 6 { | ||
| + cpn, err = strconv.Atoi(fields[5]) | ||
| + if err != nil { | ||
| + return e, fmt.Errorf("cannot parse check pass number: %s", err) |
zyga commentedMar 29, 2017
This patch adds a simple function for parsing fstab-like mount entries
along with full suite of tests.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com