Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
errtracker: try multiple paths to read machine-id #3337
Conversation
| + // but we have /etc/machine-id. See | ||
| + // https://www.freedesktop.org/software/systemd/man/machine-id.html for a | ||
| + // few more details. | ||
| + machineIDs = []string{"/var/lib/dbus/machine-id", "/etc/machine-id"} |
morphis
May 17, 2017
Contributor
We could alternatively default to read /etc/machine-id on all systems but I am not sure why /var/lib/dbus/machine-id was preferred over /etc/machine-id. Maybe because of 14.04 support?
| + // but we have /etc/machine-id. See | ||
| + // https://www.freedesktop.org/software/systemd/man/machine-id.html for a | ||
| + // few more details. | ||
| + machineIDs = []string{"/var/lib/dbus/machine-id", "/etc/machine-id"} |
zyga
May 17, 2017
Contributor
I think the order should be the other way around. We should prefer /etc/machine-id
morphis
May 17, 2017
Contributor
We can do that but I wanted to preserve what we did so far and keep reporting these ids. If everyone agrees I am happy to change this.
zyga
May 17, 2017
Contributor
According to the manual page /etc/machine-id should take priority so I'd go with that.
| + var err error | ||
| + for _, id := range machineIDs { | ||
| + machineID, err = ioutil.ReadFile(id) | ||
| + if err == nil { |
zyga
May 17, 2017
Contributor
I think we should only silently ignore the error when the file does not exist and log all other errors.
mvo5
May 18, 2017
Collaborator
One more nitpick/simplification suggestion:
--- a/errtracker/errtracker.go
+++ b/errtracker/errtracker.go
@@ -67,10 +67,8 @@ func distroRelease() string {
}
func readMachineID() ([]byte, error) {
- var machineID []byte
- var err error
for _, id := range machineIDs {
- machineID, err = ioutil.ReadFile(id)
+ machineID, err := ioutil.ReadFile(id)
if err == nil {
return bytes.TrimSpace(machineID), nil
} else if !os.IsNotExist(err) {
| } | ||
| + | ||
| + if len(machineID) == 0 { | ||
| + return "", fmt.Errorf("Can not report as no machine id found") |
zyga
May 17, 2017
Contributor
We always use "cannot" in error messages.
cannot report error: no suitable machine-id file was found
mvo5
approved these changes
May 17, 2017
Looks very nice, one small suggestion about how to make the code slightly nicer.
| - return "", err | ||
| + var machineID []byte | ||
| + var err error | ||
| + for _, id := range machineIDs { |
mvo5
May 17, 2017
Collaborator
nitpick: I think this would be slightly nicer if extracted into a helper like func readMachineID() ([]byte, error).
added some commits
May 17, 2017
All checks have passed
codecov-io
commented
May 18, 2017
•
Edited 1 time
-
codecov-io
May 18, 2017
-
May 18, 2017
Codecov Report
@@ Coverage Diff @@
## master #3337 +/- ##
==========================================
- Coverage 77.58% 77.58% -0.01%
==========================================
Files 364 364
Lines 24973 24982 +9
==========================================
+ Hits 19376 19382 +6
- Misses 3870 3872 +2
- Partials 1727 1728 +1
Continue to review full report at Codecov.
|
| + for _, id := range machineIDs { | ||
| + machineID, err = ioutil.ReadFile(id) | ||
| + if err == nil { | ||
| + break |
mvo5
May 18, 2017
Collaborator
You can simplify this code by returning here: return bytes.TrimSpace(machineID), nil
Then you can write after the loop: return nil, fmt.Errorf("cannot report: no suitable machine id file found") and avoid the len(machineID) check altogether.
morphis commentedMay 17, 2017
The machine-id file is at different locations depending on how the system
is setup. On Fedora for example /var/lib/dbus/machine-id doesn't exist
but we have /etc/machine-id. See
https://www.freedesktop.org/software/systemd/man/machine-id.html for a
few more details.