Skip to content

Commit

Permalink
ftrace: Properly parse comm (process name) even if it has spaces
Browse files Browse the repository at this point in the history 
bug 579234

If a thread name has spaces the space is incorrectly interpreted as
a separator for key-value pairs, therefore ignoring the part after
the space,

When parsing fields look for the pattern "<key>=" and take as value
whatever string between the key pattern and the next key pattern,
ignoring trailing '[', ']' or ',' characters.

This under the assumption that the key does not contain spaces and
the value does not contain '='.

Added a unit test for this case.

Change-Id: I5836169e19156ebcb51f2c58bd6c6352df589cec
Signed-off-by: Fabrizio Iannetti <fabrizio.iannetti@gmail.com>
Reviewed-on: https://git.eclipse.org/r/c/tracecompass.incubator/org.eclipse.tracecompass.incubator/+/191818
Tested-by: Trace Compass Bot <tracecompass-bot@eclipse.org>
Tested-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Reviewed-by: Hoang Thuan Pham <hoangpham.eclipse@gmail.com>
Reviewed-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Reviewed-by: Bernd Hufmann <bernd.hufmann@ericsson.com>
  • Loading branch information
@fabrizioiannetti @MatthewKhouzam
fabrizioiannetti authored and MatthewKhouzam committed Mar 28, 2022
1 parent 370c4de commit cc3de14
Show file tree
Hide file tree
Showing 2 changed files with 82 additions and 27 deletions.
26 changes: 26 additions & 0 deletions ...tests/src/org/eclipse/tracecompass/incubator/ftrace/core/tests/event/FtraceFieldTest.java
View file
Expand Up @@ -52,6 +52,32 @@ public void testParseSchedWakeupLine() {
assertEquals((Long) 0L, field.getContent().getFieldValue(Long.class, "target_cpu"));
}

/**
* Testing of parse line with function using line from an ftrace output where
* the command name contains a space (comm=daemo su), check that the parsed
* comm field value is the complete name, including the space.
*/
@Test
public void testParseEventWithCommPropertyWithSpace() {
String line = "kworker/0:0-9514 [000] d..4 3210.263482: sched_wakeup: comm=daemo su pid=16620 prio=120 success=1 target_cpu=000";

GenericFtraceField field = GenericFtraceField.parseLine(line);

assertNotNull(field);
assertEquals((Integer) 0, field.getCpu());
assertEquals((Integer) 9514, field.getPid());
assertEquals((Integer) 9514, field.getTid());
assertEquals(3210263482000L, (long) field.getTs());
assertEquals("sched_wakeup", field.getName());

assertEquals(5, field.getContent().getFields().size());
assertEquals("daemo su", field.getContent().getFieldValue(String.class, "comm"));
assertEquals((Long) 1L, field.getContent().getFieldValue(Long.class, "success"));
assertEquals((Long) 16620L, field.getContent().getFieldValue(Long.class, "pid"));
assertEquals((Long) 120L, field.getContent().getFieldValue(Long.class, "prio"));
assertEquals((Long) 0L, field.getContent().getFieldValue(Long.class, "target_cpu"));
}

/**
* Testing of parse line with function using line from an ftrace output
*/
Expand Down
83 changes: 56 additions & 27 deletions ...src/org/eclipse/tracecompass/incubator/internal/ftrace/core/event/GenericFtraceField.java
View file
Expand Up @@ -17,6 +17,7 @@
import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.eclipse.tracecompass.analysis.os.linux.core.kernel.LinuxValues;
import org.eclipse.tracecompass.incubator.internal.ftrace.core.layout.GenericFtraceEventLayout;
import org.eclipse.tracecompass.tmf.core.event.ITmfEventField;
import org.eclipse.tracecompass.tmf.core.event.TmfEventField;

Expand All @@ -37,9 +38,11 @@
@NonNullByDefault
public class GenericFtraceField {

private static final Pattern KEYVAL_PATTERN = Pattern.compile("(?<key>[^\\s=\\[\\],]+)(=|:)\\s*(?<val>[^\\s=\\[\\],]+)"); //$NON-NLS-1$
private static final Pattern KEYVAL_KEY_PATTERN = Pattern.compile("(?<key>[^\\s=\\[\\],]+)(=|:)"); //$NON-NLS-1$
private static final Pattern KEYVAL_VALUE_PATTERN = Pattern.compile("\\s*(?<value>[^\\[\\],]+).*"); //$NON-NLS-1$
private static final String KEYVAL_KEY_GROUP = "key"; //$NON-NLS-1$
private static final String KEYVAL_VAL_GROUP = "val"; //$NON-NLS-1$
private static final String KEYVAL_VALUE_GROUP = "value"; //$NON-NLS-1$


private static final double SECONDS_TO_NANO = 1000000000.0;
private static final Map<Character, @NonNull Long> PREV_STATE_LUT;
Expand Down Expand Up @@ -129,42 +132,68 @@ public GenericFtraceField(String name, Integer cpu, Long ts, @Nullable Integer p

Map<@NonNull String, @NonNull Object> fields = new HashMap<>();

Matcher keyvalMatcher = KEYVAL_PATTERN.matcher(attributes);
while (keyvalMatcher.find()) {
String key = keyvalMatcher.group(KEYVAL_KEY_GROUP);
String value = keyvalMatcher.group(KEYVAL_VAL_GROUP);
if (value != null) {
// This is a temporary solution. Refactor suggestions are welcome.
if (key.equals("prev_state")) { //$NON-NLS-1$
fields.put(key, parsePrevStateValue(value));
} else if (StringUtils.isNumeric(value)) {
if (key.equals("parent_pid") && name.equals("sched_process_fork")) {//$NON-NLS-1$ //$NON-NLS-2$
key = "pid"; //$NON-NLS-1$
}
fields.put(key, Long.parseUnsignedLong(value));
} else {
fields.put(key, decodeString(value));
if (attributes != null && !attributes.isEmpty()) {
int valStart = 0;
Matcher keyvalMatcher = KEYVAL_KEY_PATTERN.matcher(attributes);
String key = null;
while (keyvalMatcher.find()) {
if (key != null && valStart > 0) {
String value = attributes.substring(valStart, keyvalMatcher.start());
putKeyValueField(name, fields, key, value);
}
valStart = keyvalMatcher.end();
key = keyvalMatcher.group(KEYVAL_KEY_GROUP);
}
}

/*
* If anything else fails, but we have discovered sort of a valid event
* attributes lets just add the unparsed attributes with key "data".
*/
if (fields.isEmpty() && attributes != null && !attributes.isEmpty()) {
String key = "data"; //$NON-NLS-1$
if (name.equals(IGenericFtraceConstants.FTRACE_EXIT_SYSCALL)) {
key = "ret"; //$NON-NLS-1$
if (key != null && valStart > 0) {
String value = attributes.substring(valStart, attributes.length());
putKeyValueField(name, fields, key, value);
}

/*
* If anything else fails, but we have discovered sort of a valid event
* attributes lets just add the unparsed attributes with key "data".
*/
if (fields.isEmpty()) {
key = "data"; //$NON-NLS-1$
if (name.equals(IGenericFtraceConstants.FTRACE_EXIT_SYSCALL)) {
key = "ret"; //$NON-NLS-1$
}
fields.put(key, decodeString(attributes));
}
fields.put(key, decodeString(attributes));
}

return new GenericFtraceField(name, cpu, timestampInNano, pid, tid, fields);
}
return null;
}

private static void putKeyValueField(String name, Map<@NonNull String, @NonNull Object> fields, String key, String value) {
Matcher valMatcher = KEYVAL_VALUE_PATTERN.matcher(value);
String actualValue;
if (valMatcher.matches()) {
actualValue = valMatcher.group(KEYVAL_VALUE_GROUP).trim();
} else {
actualValue = value;
}
if (!actualValue.isBlank()) {
// This is a temporary solution. Refactor suggestions
// are welcome.
final GenericFtraceEventLayout eventLayout = GenericFtraceEventLayout.getInstance();
if (key.equals(eventLayout.fieldPrevState())) {
fields.put(key, parsePrevStateValue(actualValue));
} else if (StringUtils.isNumeric(actualValue)) {
String actualKey = key;
if (key.equals("parent_pid") && name.equals(eventLayout.eventSchedProcessFork())) { //$NON-NLS-1$
actualKey = eventLayout.fieldTid();
}
fields.put(actualKey, Long.parseUnsignedLong(actualValue));
} else {
fields.put(key, decodeString(actualValue));
}
}
}

private static Object decodeString(String val) {
try {
if (val.startsWith("0x") || val.startsWith("0X")) { //$NON-NLS-1$ //$NON-NLS-2$
Expand Down

0 comments on commit cc3de14

Please sign in to comment.