-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Regexp tidy up #978
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Regexp tidy up #978
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,12 +34,17 @@ import ( | |
|
||
// The utils/machine package contains functions that extract machine-level specs. | ||
|
||
var cpuRegExp = regexp.MustCompile("processor\\t*: +([0-9]+)") | ||
var coreRegExp = regexp.MustCompile("core id\\t*: +([0-9]+)") | ||
var nodeRegExp = regexp.MustCompile("physical id\\t*: +([0-9]+)") | ||
var CpuClockSpeedMHz = regexp.MustCompile("cpu MHz\\t*: +([0-9]+.[0-9]+)") | ||
var memoryCapacityRegexp = regexp.MustCompile("MemTotal: *([0-9]+) kB") | ||
var swapCapacityRegexp = regexp.MustCompile("SwapTotal: *([0-9]+) kB") | ||
var ( | ||
cpuRegExp = regexp.MustCompile(`^processor\s*:\s*([0-9]+)$`) | ||
coreRegExp = regexp.MustCompile(`^core id\s*:\s*([0-9]+)$`) | ||
nodeRegExp = regexp.MustCompile(`^physical id\s*:\s*([0-9]+)$`) | ||
// Power systems have a different format so cater for both | ||
cpuClockSpeedMHz = regexp.MustCompile(`(?:cpu MHz|clock)\s*:\s*([0-9]+\.[0-9]+)(?:MHz)?`) | ||
memoryCapacityRegexp = regexp.MustCompile(`MemTotal:\s*([0-9]+) kB`) | ||
swapCapacityRegexp = regexp.MustCompile(`SwapTotal:\s*([0-9]+) kB`) | ||
) | ||
|
||
const maxFreqFile = "/sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_max_freq" | ||
|
||
// GetClockSpeed returns the CPU clock speed, given a []byte formatted as the /proc/cpuinfo file. | ||
func GetClockSpeed(procInfo []byte) (uint64, error) { | ||
|
@@ -49,7 +54,6 @@ func GetClockSpeed(procInfo []byte) (uint64, error) { | |
} | ||
|
||
// First look through sys to find a max supported cpu frequency. | ||
const maxFreqFile = "/sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_max_freq" | ||
if utils.FileExists(maxFreqFile) { | ||
val, err := ioutil.ReadFile(maxFreqFile) | ||
if err != nil { | ||
|
@@ -63,15 +67,11 @@ func GetClockSpeed(procInfo []byte) (uint64, error) { | |
return maxFreq, nil | ||
} | ||
// Fall back to /proc/cpuinfo | ||
matches := CpuClockSpeedMHz.FindSubmatch(procInfo) | ||
matches := cpuClockSpeedMHz.FindSubmatch(procInfo) | ||
if len(matches) != 2 { | ||
//Check if we are running on Power systems which have a different format | ||
CpuClockSpeedMHz, _ = regexp.Compile("clock\\t*: +([0-9]+.[0-9]+)MHz") | ||
matches = CpuClockSpeedMHz.FindSubmatch(procInfo) | ||
if len(matches) != 2 { | ||
return 0, fmt.Errorf("could not detect clock speed from output: %q", string(procInfo)) | ||
} | ||
return 0, fmt.Errorf("could not detect clock speed from output: %q", string(procInfo)) | ||
} | ||
|
||
speed, err := strconv.ParseFloat(string(matches[1]), 64) | ||
if err != nil { | ||
return 0, err | ||
|
@@ -139,6 +139,9 @@ func GetTopology(sysFs sysfs.SysFs, cpuinfo string) ([]info.Node, int, error) { | |
lastCore := -1 | ||
lastNode := -1 | ||
for _, line := range strings.Split(cpuinfo, "\n") { | ||
if line == "" { | ||
continue | ||
} | ||
ok, val, err := extractValue(line, cpuRegExp) | ||
if err != nil { | ||
return nil, -1, fmt.Errorf("could not parse cpu info from %q: %v", line, err) | ||
|
@@ -157,20 +160,23 @@ func GetTopology(sysFs sysfs.SysFs, cpuinfo string) ([]info.Node, int, error) { | |
lastNode = -1 | ||
} | ||
lastThread = thread | ||
continue | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This whole loop could use some cleanup. I think it would be much more efficient and readable to switch on That's sort of a separate change, so up to you where / whether you want to address it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. Not going to do it here, luckily AFAICT this is only called once on start of cadvisor so efficiency isn't paramount. Would like to improve it though, for vanity more than anything else :-b |
||
} | ||
ok, val, err = extractValue(line, coreRegExp) | ||
if err != nil { | ||
return nil, -1, fmt.Errorf("could not parse core info from %q: %v", line, err) | ||
} | ||
if ok { | ||
lastCore = val | ||
continue | ||
} | ||
ok, val, err = extractValue(line, nodeRegExp) | ||
if err != nil { | ||
return nil, -1, fmt.Errorf("could not parse node info from %q: %v", line, err) | ||
} | ||
if ok { | ||
lastNode = val | ||
continue | ||
} | ||
} | ||
nodeIdx, err := addNode(&nodes, lastNode) | ||
|
@@ -213,7 +219,7 @@ func extractValue(s string, r *regexp.Regexp) (bool, int, error) { | |
if len(matches) == 2 { | ||
val, err := strconv.ParseInt(string(matches[1]), 10, 32) | ||
if err != nil { | ||
return true, -1, err | ||
return false, -1, err | ||
} | ||
return true, int(val), nil | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,7 +24,7 @@ import ( | |
"github.com/google/cadvisor/utils/sysfs" | ||
) | ||
|
||
var schedulerRegExp = regexp.MustCompile(".*\\[(.*)\\].*") | ||
var schedulerRegExp = regexp.MustCompile(`.*\[(.*)\].*`) | ||
|
||
// Get information about block devices present on the system. | ||
// Uses the passed in system interface to retrieve the low level OS information. | ||
|
@@ -65,18 +65,14 @@ func GetBlockDeviceInfo(sysfs sysfs.SysFs) (map[string]info.DiskInfo, error) { | |
// size is in 512 bytes blocks. | ||
disk_info.Size = size * 512 | ||
|
||
sched, err := sysfs.GetBlockDeviceScheduler(name) | ||
if err != nil { | ||
sched = "none" | ||
} else { | ||
matches := schedulerRegExp.FindSubmatch([]byte(sched)) | ||
if len(matches) < 2 { | ||
sched = "none" | ||
} else { | ||
sched = string(matches[1]) | ||
disk_info.Scheduler = "none" | ||
blkSched, err := sysfs.GetBlockDeviceScheduler(name) | ||
if err == nil { | ||
matches := schedulerRegExp.FindSubmatch([]byte(blkSched)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This regex should use a raw string too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
if len(matches) >= 2 { | ||
disk_info.Scheduler = string(matches[1]) | ||
} | ||
} | ||
disk_info.Scheduler = sched | ||
device := fmt.Sprintf("%d:%d", disk_info.Major, disk_info.Minor) | ||
diskMap[device] = disk_info | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why move this out of the function? Personally I find it more readable if I can see the regexp right where it's used (and I don't think this should be used outside that function).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting it in the function means the RE gets wastefully compiled every time the function is called. In this case probably not called more than once, but normally best to move it outside so trying to keep it consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, makes sense. Maybe move the declaration to just above the function in that case? Also, consider making the string const.