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 parser for mountinfo entries #3183
Conversation
All checks have passed
chipaca
approved these changes
Apr 12, 2017
Seems alright, although I think the errors could use improvement.
| + // Parse MountID (decimal number). | ||
| + e.MountID, err = strconv.Atoi(fields[0]) | ||
| + if err != nil { | ||
| + return e, fmt.Errorf("cannot parse mount ID: %q", fields[0]) |
chipaca
Apr 12, 2017
Member
"cannot parse mount ID %q: %v", fields[0], err? Maybe even err.(*strconv.NumError).Err instead of just err. The first would result in cannot parse mount ID "0x3": strconv.ParseInt: parsing "0x3": invalid syntax, which is too verbose, but the second is just right imho: cannot parse mount ID "0x3": invalid syntax. Note strconv says ParseInt errors will be *strconv.NumError (but I'd still check)
zyga
Apr 12, 2017
Contributor
The errors from strconv are terribly useless. We did this exercise in fstab parser. I honestly think that any human being can see if this is not a number.
| + // Parse ParentID (decimal number). | ||
| + e.ParentID, err = strconv.Atoi(fields[1]) | ||
| + if err != nil { | ||
| + return e, fmt.Errorf("cannot parse parent mount ID: %q", fields[1]) |
stolowski
requested changes
Apr 12, 2017
Looks good to me, just one comment re OptionalFields below.
| + if i == len(fields) { | ||
| + return e, fmt.Errorf("list of optional fields is not terminated properly") | ||
| + } | ||
| + e.OptionalFlds = strings.Join(fields[6:i], " ") |
stolowski
Apr 12, 2017
Contributor
It seems a bit wasteful to concatenate optional fields back. Even if they are not going to be useful at the moment, it's imho ok to just keep them as an array.
zyga
Apr 12, 2017
Contributor
I would like to send a follow up branch that focuses on field renames and deeper parsing. If you revise your decision we can land this now (not wasting CI efforts) and I can follow up with []string OptionalFields and map[string]string MountOptions and similar SuperBlockOptions
stolowski
Apr 12, 2017
Contributor
@zyga Sure, np. Also, feel free not to parse fields we don't need. In this particular case it just feels concatenating is unnecessary; I'm fine if you address this in a followup branch.
zyga commentedApr 12, 2017
This patch adds a parser for mountinfo entries. Optional fields as well
as various mount options are not parsed deeeply but that is not required
yet.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com