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.

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

ta: cleanup trace messages in test TAs #710

Merged
merged 4 commits into from
Dec 8, 2023
Merged

Conversation

etienne-lms
Copy link
Contributor

This is a follow up of cleanup changes initiated in #708 and #709 to have TA trace messages implementation more consistent with OP-TEE coding style and trace resources.

Copy link
Contributor

@jbech-linaro jbech-linaro left a comment

Choose a reason for hiding this comment

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

Reviewed-by: Joakim Bech <[email protected]>

Copy link
Contributor

@vincent-mailhol vincent-mailhol left a comment

Choose a reason for hiding this comment

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

Global comments: in the commit description, the best practise is to use the imperative tone. This means no third person "s" in the verbs. e.g. s/Uses/Use/g, s/Adds/Add/g, s/Removes/Remove/g and so on.

Reference: https://www.kernel.org/doc/html/v4.10/process/submitting-patches.html#describe-your-changes

Describe your changes in imperative mood, e.g. “make xyzzy do frotz” instead of “[This patch] makes xyzzy do frotz” or “[I] changed xyzzy to do frotz”, as if you are giving orders to the codebase to change its behaviour.

Comment on lines 25 to 26
EMSG("\n*** FAILED ***" \
"\nTestbench halted at line %d in function %s"; \
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
EMSG("\n*** FAILED ***" \
"\nTestbench halted at line %d in function %s"; \
EMSG("\n*** FAILED ***" \
"\nTestbench halted at line %d in function %s"; \

Fix the alignment.

@etienne-lms
Copy link
Contributor Author

Thanks @vincent-mailhol for the review.

Global comments: in the commit description, the best practise is to use the imperative tone. (...)

@jforissier, @jbech-linaro, is this the preferred phrasing in OP-TEE repos commits?

@jforissier
Copy link
Contributor

Thanks @vincent-mailhol for the review.

Global comments: in the commit description, the best practise is to use the imperative tone. (...)

@jforissier, @jbech-linaro, is this the preferred phrasing in OP-TEE repos commits?

Yes.

@etienne-lms
Copy link
Contributor Author

comments addressed.
@jbech-linaro, could you infirm or confirm your review tag?

@etienne-lms
Copy link
Contributor Author

@vincent-mailhol, maybe you want to post a review tag for this series?

@vincent-mailhol
Copy link
Contributor

@vincent-mailhol, maybe you want to post a review tag for this series?

I am fine with the contents. But my main comment is on the commit description and I do not see changes. This is why I have not given my review tag yet.

@etienne-lms
Copy link
Contributor Author

Ok, thanks for the feedback . I'll squash the fixup commits right away.

@etienne-lms
Copy link
Contributor Author

Done

@vincent-mailhol
Copy link
Contributor

For the series:

Reviewed-by: Vincent Mailhol <[email protected]>

Thank you.

@jbech-linaro
Copy link
Contributor

@jbech-linaro, could you infirm or confirm your review tag?

indeed!

@jbech-linaro
Copy link
Contributor

PR probably needs a rebase as well, since we see the pkcs11 1007 error here as well.

Use PRI* specifiers in trace messages using xMSG() macros.

Add parentheses to explicit function labels.

Remove function label from EMSG() trace messages as the macro already
takes care of printing it.

Reviewed-by: Joakim Bech <[email protected]>
Reviewed-by: Vincent Mailhol <[email protected]>
Signed-off-by: Etienne Carriere <[email protected]>
Use PRI* specifiers in trace messages using xMSG() macros and fix
indentation issues.

Reviewed-by: Joakim Bech <[email protected]>
Reviewed-by: Vincent Mailhol <[email protected]>
Signed-off-by: Etienne Carriere <[email protected]>
Use PRI* specifiers in trace messages using xMSG() macros and fix
indentation issues.

Reviewed-by: Joakim Bech <[email protected]>
Reviewed-by: Vincent Mailhol <[email protected]>
Signed-off-by: Etienne Carriere <[email protected]>
Replace printf() occurrences with use of MSG() macro in TA
implementation source and header files. MSG() macro already prints
the function label and the line number hence remove these explicit
arguments from trace messages printed by HALT() macro.

Use PRI* specifiers in trace messages using xMSG() macros.

Add parentheses to function names in trace message where applicable
to explicit these are function labels.

Add an empty line in implementation of DEF_BIGINT() macro to comply
with OP-TEE coding style.

Reviewed-by: Joakim Bech <[email protected]>
Reviewed-by: Vincent Mailhol <[email protected]>
Signed-off-by: Etienne Carriere <[email protected]>
@etienne-lms
Copy link
Contributor Author

Rebased. IBART test seems ok. Thanks.
Review tags applied.

@etienne-lms
Copy link
Contributor Author

ok to merge?

@jforissier jforissier merged commit 63d4b58 into OP-TEE:master Dec 8, 2023
1 check passed
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.

4 participants