Skip to content
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.

Sign up for GitHub

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

ASoC: SOF: Fix skipping empty trace filters #2516

Merged
merged 1 commit into from
Nov 9, 2020
Merged

ASoC: SOF: Fix skipping empty trace filters #2516

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 3 additions & 2 deletions sound/soc/sof/trace.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,9 @@ static int trace_filter_parse_entry(struct snd_sof_dev *sdev, const char *line,
int ret;

/* ignore empty content */
if (!line[0])
return 0;
sscanf(line, " %n", &read);
ktrzcinx marked this conversation as resolved.
Show resolved Hide resolved
if (read == len)
return len;

ret = sscanf(line, " %d %x %d %d %n", &log_level, &uuid_id, &pipe_id, &comp_id, &read);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ktrzcinx Hmm, the sscanf just to read whitespace seems a bit odd. How about if we consume the linefeed in the existing sscanf calll on L51. Would this work?

Copy link
Member Author

@ktrzcinx ktrzcinx Oct 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

sscanf just to read whitespace

rather to check if input string consist only from whitespaces or is empty one, so its eg one of "\0", "\n\0", " \0", " \n\0".

seems a bit odd

I think it shortest and reliable solution as long as reader are familiar with scanf() format string. I didn't use any hack here, it's just standard function usage.

How about if we consume the linefeed in the existing sscanf calll on L51. Would this work?

There already is linefeed consumer in the existing sscanf calll on L51 and it doesn't work in all situations 😄. The case here is parsing last entry of strsep("0 0 0 0;\n", ";"), it will be "\n", so scanning 4 numbers fails for it. Without this fix it works for strsep("0 0 0 0;", ";"), because then last entry is "\0" iirc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ktrzcinx Thanks. I agree this is probably the cleanest, but I'm still not entirely convinced having this sort of relaxed parsing in kernel space is the right place. I mean "0 0 0 0;\n" is an invalid input in the end. Why have extra code in kernel for something user-space can easily just get right.

Copy link
Member Author

Choose a reason for hiding this comment

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

Without this change we have some inconsistence - "0 0 0 0;\n" returns parsing error but "0 0 0 0\n" passes, after change it will be consistent.

Copy link
Member

Choose a reason for hiding this comment

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

I get the point @ktrzcinx about detecting a line with whitespaces only, but I don't get how this change helps you detect
"0 0 0 0;\n" from "0 0 0 0\n"

Copy link
Member Author

Choose a reason for hiding this comment

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

@plbossart "0 0 0 0;\n" will be split to "0 0 0 0\0" (parsed with success) and "\n" (rising parse error without this patch). Such and situation doesn't happen for "0 0 0 0\n", because there's no spiting char, and this text will be consumed at once.

if (ret != TRACE_FILTER_ELEMENTS_PER_ENTRY || read != len) {
Expand Down