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

lib/vfscore: Improve and fix trace point format #899

Closed

Conversation

mschlumpp
Copy link
Member

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran the checkpatch.pl on your commit series before opening this PR;
  • Updated relevant documentation.

Base target

  • Architecture(s): N/A
  • Platform(s): N/A
  • Application(s): N/A

Additional configuration

  • CONFIG_LIBVFSCORE=y

Description of changes

There were multiple issues with the format string used in the trace points. For example python, which is used to interpret the resulting trace files, doesn not support the %p specifier and we instead have to use %x. Also we can make use of the alternate format (e.g. %#x) to ensure the hexadecimal numbers have a 0x prefixed. Some format strings also did not use the correct amount of arguments.

@mschlumpp mschlumpp requested a review from a team as a code owner May 17, 2023 07:37
@razvand razvand requested review from Starnox and eduardvintila and removed request for a team May 17, 2023 07:40
@razvand razvand self-assigned this May 17, 2023
@razvand razvand added lib/vfscore VFS Core Interface area/lib Internal Unikraft Microlibrary kind/quick-fix Issue is a quick fix labels May 17, 2023
@razvand razvand added this to the v0.14.0 (Prometheus) milestone May 17, 2023
@unikraft-bot
Copy link
Member

Checkpatch passed

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request and it all looks good!

SHA commit checkpatch
f75a5ba lib/vfscore: Improve and fix trace point format

@razvand
Copy link
Contributor

razvand commented Jun 1, 2023

@mschlumpp, you mention For example python, which is used to interpret the resulting trace files, doesn not support the %pspecifier and we instead have to use%x. Why is the format (%p) relevant for Python post-processing? The results are going to be printed in hexadecimal format, according to the %p` interpretation of the C print implementation.

Also, see the doesn typo.

@mschlumpp
Copy link
Member Author

@mschlumpp, you mention [...]. Why is the format (%p) relevant for Python post-processing? The results are going to be printed in hexadecimal format, according to the %p interpretation of the C print implementation.

The interpolation happens in the post-processing phase (Python). Therefore, we are limited in the format strings what Python supports.

@mschlumpp mschlumpp force-pushed the mschlumpp/fix/vfscore-tracing branch from f75a5ba to 5d285fd Compare June 21, 2023 15:28
@razvand razvand added the size/small Small PR, quick to review label Jun 28, 2023
Copy link
Contributor

@Starnox Starnox left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Reviewed-by: Eduard-Florin Mihailescu mihailescu.eduard@gmail.com

Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Approved-by: Razvan Deaconescu razvand@unikraft.io

There were multiple issues with the format string used in the trace
points. For example, python, which is used to interpret the resulting
trace files, does not support the `%p` specifier, and we instead have to
use `%x`. Also, we can make use of the alternate format (e.g. `%#x`) to
ensure the hexadecimal numbers have a `0x` prefixed. Some format strings
also did not use the correct number of arguments.

Signed-off-by: Marco Schlumpp <marco@unikraft.io>
@mschlumpp mschlumpp force-pushed the mschlumpp/fix/vfscore-tracing branch from 5d285fd to b178c16 Compare August 9, 2023 10:35
Copy link
Contributor

@razvand razvand 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: Razvan Deaconescu razvand@unikraft.io
Approved-by: Razvan Deaconescu razvand@unikraft.io

@unikraft-bot unikraft-bot added the ci/merged Merged by CI label Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/lib Internal Unikraft Microlibrary ci/merged Merged by CI kind/quick-fix Issue is a quick fix lib/vfscore VFS Core Interface size/small Small PR, quick to review
Projects
Status: Done!
Development

Successfully merging this pull request may close these issues.

None yet

4 participants