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

Fix: no newline when debug msg over DEBUG_BUF_SIZE #6511

Closed
wants to merge 8 commits into from
Closed

Fix: no newline when debug msg over DEBUG_BUF_SIZE #6511

wants to merge 8 commits into from

Conversation

valord577
Copy link
Contributor

@valord577 valord577 commented Oct 31, 2022

Signed-off-by: valord577 valord577@gmail.com

Description

Fix: no newline when debug msg over DEBUG_BUF_SIZE

Status

READY/IN

!!! Please Use Squash Merging

Signed-off-by: valord577 <valord577@gmail.com>
@daverodgman daverodgman added bug needs-review Every commit must be reviewed by at least two team members, component-platform Portability layer and build scripts needs-reviewer This PR needs someone to pick it up for review size-s Estimated task size: small (~2d) priority-medium Medium priority - this can be reviewed as time permits labels Oct 31, 2022
library/debug.c Outdated Show resolved Hide resolved
Fix trailing white-space

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>
@daverodgman daverodgman added the needs-ci Needs to pass CI tests label Oct 31, 2022
library/debug.c Outdated Show resolved Hide resolved
library/debug.c Outdated Show resolved Hide resolved
library/debug.c Outdated Show resolved Hide resolved
Signed-off-by: valord577 <valord577@gmail.com>
Signed-off-by: valord577 <valord577@gmail.com>
library/debug.c Outdated Show resolved Hide resolved
library/debug.c Outdated Show resolved Hide resolved
Signed-off-by: valord577 <valord577@gmail.com>
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

LGTM

daverodgman
daverodgman previously approved these changes Feb 15, 2023
Copy link
Contributor

@daverodgman daverodgman left a comment

Choose a reason for hiding this comment

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

LGTM. I would ideally like to see a comment next to line 33 specifying that DEBUG_BUF_SIZE must be > 2 rather than just rely on the static assert elsewhere, but it's not a blocker.

Edit: actually, rather than iterate the PR I've just added a comment. If @tom-cosgrove-arm approves we can merge.

@daverodgman daverodgman removed the needs-review Every commit must be reviewed by at least two team members, label Feb 15, 2023
@daverodgman daverodgman added approved Design and code approved - may be waiting for CI or backports and removed needs-reviewer This PR needs someone to pick it up for review needs-ci Needs to pass CI tests labels Feb 15, 2023
Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>
@daverodgman daverodgman dismissed stale reviews from tom-cosgrove-arm and themself via 803658e February 15, 2023 17:41
@daverodgman daverodgman added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests and removed approved Design and code approved - may be waiting for CI or backports labels Feb 15, 2023
@valord577
Copy link
Contributor Author

:p - Please Use Squash Merging!

Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@tom-cosgrove-arm
Copy link
Contributor

Does this need a backport?

@tom-cosgrove-arm tom-cosgrove-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Feb 15, 2023
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

  • Missing #include <assert.h>
  • Please backport to mbedtls-2.28

@@ -69,6 +70,10 @@ void mbedtls_debug_print_msg(const mbedtls_ssl_context *ssl, int level,
char str[DEBUG_BUF_SIZE];
int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;

#if defined(static_assert)
Copy link
Contributor

Choose a reason for hiding this comment

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

That's never going to trigger without #include <assert.h>.

Copy link
Contributor

Choose a reason for hiding this comment

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

Bah, good spot - thanks

@gilles-peskine-arm gilles-peskine-arm added needs-work and removed approved Design and code approved - may be waiting for CI or backports needs-ci Needs to pass CI tests labels Feb 28, 2023
@daverodgman daverodgman added the historical-reviewing Currently reviewing (for legacy PR/issues) label May 12, 2023
@daverodgman
Copy link
Contributor

Rebased with a fix for the static assert issue in #7607

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component-platform Portability layer and build scripts historical-reviewing Currently reviewing (for legacy PR/issues) needs-work priority-medium Medium priority - this can be reviewed as time permits size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants