Skip to content

Conversation

@xiulipan
Copy link
Contributor

@xiulipan xiulipan commented Nov 6, 2020

What we need is to set a checkpoint for kernel log to let
us start check kernel. Rename last_line to a more generic
name checkpoint.

Signed-off-by: Pan Xiuli xiuli.pan@linux.intel.com

@xiulipan xiulipan requested a review from a team as a code owner November 6, 2020 05:43
@xiulipan
Copy link
Contributor Author

xiulipan commented Nov 6, 2020

@marc-hb Done, please have a look or blind approval.

@xiulipan
Copy link
Contributor Author

xiulipan commented Nov 9, 2020

@marc-hb Ping

marc-hb
marc-hb previously approved these changes Nov 9, 2020
Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

Thanks, the change is good and the name "checkpoint" sounds great but the commit message is a bit incomplete: It's not obvious why sof-kernel-log-check.sh $VAR is the same as sof-kernel-log-check.sh "$VAR" and the commit message should mention why they have the same effect in this particular codebase.

sof-kernel-log-check.sh $VAR is the same as sof-kernel-log-check.sh "$VAR" only because sof-kernel-log-check.sh starts with ${1:-1}.

If sof-kernel-log-check.sh were for instance starting with ${1-1} then sof-kernel-log-check.sh $VAR is the same as sof-kernel-log-check.sh "$VAR" would be a real functional change.

In general one cannot assume that $VAR is the same as "$VAR"`. This difference is not a purely theoretical issue, see real example in #430 (review) . Shellcheck warnings are rarely just about "codestyle" , most shellcheck warnings are about functional changes.

What we need is to set a checkpoint for kernel log to let
us start check kernel. Rename last_line to a more generic
name checkpoint.
Also add quote to all function call for sof-kernel-log-check.sh
to unify the stlye.

Signed-off-by: Pan Xiuli <xiuli.pan@linux.intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants