Skip to content

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

Merged
merged 1 commit into from
Nov 26, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func RegisterHandlers(mux httpmux.Mux, m manager.Manager) error {
}

// Captures the API version, requestType [optional], and remaining request [optional].
var apiRegexp = regexp.MustCompile("/api/([^/]+)/?([^/]+)?(.*)")
var apiRegexp = regexp.MustCompile(`/api/([^/]+)/?([^/]+)?(.*)`)

const (
apiVersion = iota + 1
Expand Down
7 changes: 5 additions & 2 deletions container/docker/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,9 +172,12 @@ func (self *dockerFactory) DebugInfo() map[string][]string {
return map[string][]string{}
}

var (
version_regexp_string = `(\d+)\.(\d+)\.(\d+)`
version_re = regexp.MustCompile(version_regexp_string)
Copy link
Contributor

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).

Copy link
Collaborator Author

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.

Copy link
Contributor

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.

)

func parseDockerVersion(full_version_string string) ([]int, error) {
version_regexp_string := "(\\d+)\\.(\\d+)\\.(\\d+)"
version_re := regexp.MustCompile(version_regexp_string)
matches := version_re.FindAllStringSubmatch(full_version_string, -1)
if len(matches) != 1 {
return nil, fmt.Errorf("version string \"%v\" doesn't match expected regular expression: \"%v\"", full_version_string, version_regexp_string)
Expand Down
27 changes: 15 additions & 12 deletions container/libcontainer/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"io/ioutil"
"os"
"path"
"regexp"
"strconv"
"strings"
"time"
Expand Down Expand Up @@ -202,8 +201,6 @@ func scanTcpStats(tcpStatsFile string) (info.TcpStat, error) {
return stats, fmt.Errorf("failure opening %s: %v", tcpStatsFile, err)
}

tcpStatLineRE, _ := regexp.Compile("[0-9:].*")

tcpStateMap := map[string]uint64{
"01": 0, //ESTABLISHED
"02": 0, //SYN_SENT
Expand All @@ -223,18 +220,24 @@ func scanTcpStats(tcpStatsFile string) (info.TcpStat, error) {

scanner.Split(bufio.ScanLines)

for scanner.Scan() {
// Discard header line
b := scanner.Scan()
if !b {
return stats, scanner.Err()
}

for scanner.Scan() {
line := scanner.Text()
//skip header
matched := tcpStatLineRE.MatchString(line)

if matched {
state := strings.Fields(line)
//#file header tcp state is the 4 filed:
//sl local_address rem_address st tx_queue rx_queue tr tm->when retrnsmt uid timeout inode
tcpStateMap[state[3]]++

state := strings.Fields(line)
// TCP state is the 4th field.
// Format: sl local_address rem_address st tx_queue rx_queue tr tm->when retrnsmt uid timeout inode
tcpState := state[3]
_, ok := tcpStateMap[tcpState]
if !ok {
return stats, fmt.Errorf("invalid TCP stats line: %v", line)
}
tcpStateMap[tcpState]++
}

stats = info.TcpStat{
Expand Down
4 changes: 2 additions & 2 deletions fs/fs.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@ import (
"github.com/golang/glog"
)

var partitionRegex = regexp.MustCompile("^(:?(:?s|xv)d[a-z]+\\d*|dm-\\d+)$")

const (
LabelSystemRoot = "root"
LabelDockerImages = "docker-images"
Expand Down Expand Up @@ -224,6 +222,8 @@ func (self *RealFsInfo) GetFsInfoForPath(mountSet map[string]struct{}) ([]Fs, er
return filesystems, nil
}

var partitionRegex = regexp.MustCompile(`^(?:(?:s|xv)d[a-z]+\d*|dm-\d+)$`)

func getDiskStatsMap(diskStatsFile string) (map[string]DiskStats, error) {
diskStatsMap := make(map[string]DiskStats)
file, err := os.Open(diskStatsFile)
Expand Down
4 changes: 2 additions & 2 deletions integration/common/gce.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ import (
var zone = flag.String("zone", "us-central1-f", "Zone the instances are running in")
var project = flag.String("project", "", "Project the instances are running in")

var gceInternalIpRegexp = regexp.MustCompile(" +networkIP: +([0-9.:]+)\n")
var gceExternalIpRegexp = regexp.MustCompile(" +natIP: +([0-9.:]+)\n")
var gceInternalIpRegexp = regexp.MustCompile(`\s+networkIP:\s+([0-9.:]+)\n`)
var gceExternalIpRegexp = regexp.MustCompile(`\s+natIP:\s+([0-9.:]+)\n`)

// Gets the IP of the specified GCE instance.
func GetGceIp(hostname string) (string, error) {
Expand Down
2 changes: 1 addition & 1 deletion manager/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ import (
// Housekeeping interval.
var HousekeepingInterval = flag.Duration("housekeeping_interval", 1*time.Second, "Interval between container housekeepings")

var cgroupPathRegExp = regexp.MustCompile(".*devices.*:(.*?)[,;$].*")
var cgroupPathRegExp = regexp.MustCompile(`.*devices.*:(.*?)[,;$].*`)

// Decay value used for load average smoothing. Interval length of 10 seconds is used.
var loadDecay = math.Exp(float64(-1 * (*HousekeepingInterval).Seconds() / 10))
Expand Down
7 changes: 5 additions & 2 deletions metrics/prometheus_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,11 @@ func (p testSubcontainersInfoProvider) SubcontainersInfo(string, *info.Container
}, nil
}

var (
includeRe = regexp.MustCompile(`^(?:(?:# HELP |# TYPE)container_|cadvisor_version_info\{)`)
ignoreRe = regexp.MustCompile(`^container_last_seen\{`)
)

func TestPrometheusCollector(t *testing.T) {
prometheus.MustRegister(NewPrometheusCollector(testSubcontainersInfoProvider{}, nil))

Expand All @@ -171,8 +176,6 @@ func TestPrometheusCollector(t *testing.T) {
// (https://github.com/prometheus/client_golang/issues/58), we simply compare
// verbatim text-format metrics outputs, but ignore certain metric lines
// whose value depends on the current time or local circumstances.
includeRe := regexp.MustCompile("^(?:(?:# HELP |# TYPE)container_|cadvisor_version_info{)")
ignoreRe := regexp.MustCompile("^container_last_seen{")
for i, want := range wantLines {
if !includeRe.MatchString(want) || ignoreRe.MatchString(want) {
continue
Expand Down
36 changes: 21 additions & 15 deletions utils/machine/machine.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -157,20 +160,23 @@ func GetTopology(sysFs sysfs.SysFs, cpuinfo string) ([]info.Node, int, error) {
lastNode = -1
}
lastThread = thread
continue
Copy link
Contributor

Choose a reason for hiding this comment

The 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 strings.TrimSpace(line[:strings.Index(line, ":")-1])

That's sort of a separate change, so up to you where / whether you want to address it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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)
Expand Down Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion utils/machine/topology_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ func TestTopology(t *testing.T) {
sysFs.SetCacheInfo(c)
topology, numCores, err := GetTopology(sysFs, string(testcpuinfo))
if err != nil {
t.Errorf("failed to get topology for sample cpuinfo %s", string(testcpuinfo))
t.Errorf("failed to get topology for sample cpuinfo %s: %v", string(testcpuinfo), err)
}

if numCores != 12 {
Expand Down
11 changes: 5 additions & 6 deletions utils/oomparser/oomparser.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,11 @@ import (
"github.com/google/cadvisor/utils"
)

var containerRegexp *regexp.Regexp = regexp.MustCompile(
`Task in (.*) killed as a result of limit of (.*)`)
var lastLineRegexp *regexp.Regexp = regexp.MustCompile(
`(^[A-Z]{1}[a-z]{2} .*[0-9]{1,2} [0-9]{1,2}:[0-9]{2}:[0-9]{2}) .* Killed process ([0-9]+) \(([0-9A-Za-z_]+)\)`)
var firstLineRegexp *regexp.Regexp = regexp.MustCompile(
`invoked oom-killer:`)
var (
containerRegexp = regexp.MustCompile(`Task in (.*) killed as a result of limit of (.*)`)
lastLineRegexp = regexp.MustCompile(`(^[A-Z][a-z]{2} .*[0-9]{1,2} [0-9]{1,2}:[0-9]{2}:[0-9]{2}) .* Killed process ([0-9]+) \(([\w]+)\)`)
firstLineRegexp = regexp.MustCompile(`invoked oom-killer:`)
)

// struct to hold file from which we obtain OomInstances
type OomParser struct {
Expand Down
18 changes: 7 additions & 11 deletions utils/sysinfo/sysinfo.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This regex should use a raw string too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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
}
Expand Down