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

Separate tests to verify max expected log message size #6016

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

grom72
Copy link
Contributor

@grom72 grom72 commented Feb 20, 2024

This change is Reviewable

@grom72 grom72 added sprint goal This pull request is part of the ongoing sprint no changelog Add to skip the changelog check on your pull request labels Feb 20, 2024
@grom72 grom72 requested a review from janekmi February 20, 2024 13:19
Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

Reviewed 12 of 12 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @grom72)


src/core/log_internal.h line 114 at r1 (raw file):

 * If actual max message lenghth is eaqual to 408, it means we have a buffer
 * overflow
 */

Suggestion:

/* The actual maximum expected log line is 407.
 * An additional byte is used to detect buffer overflow in core_log tests.
 * If the actual max message length is equal to 408, it means we have a buffer
 * overflow.
 */

src/core/log_internal.h line 115 at r1 (raw file):

 * overflow
 */
#define CORE_LOG_MSG_MAXPRINT 408 /* maximum expected log line */

Suggestion:

/* maximum expected log line + 1 */

src/test/core_log_max/call_all.c line 25 at r1 (raw file):

 */
#undef abort
#define abort() do { ; } while(0)

I believe the #undef part is unnecessary.

  1. I don't think abort() is actually a definition in the first place but even if...
  2. ... you redefine it with the proceeding #define.

Suggestion:

#define abort() do { ; } while(0)

src/test/core_log_max/core_log_max.c line 179 at r1 (raw file):

	call_all_CORE_LOG_ERROR();
	call_all_CORE_LOG_ERROR_W_ERRNO(MAX_STRERROR_NUM);
	call_all_CORE_LOG_FATAL();

You missed one.

Suggestion:

	call_all_CORE_LOG_FATAL();
	call_all_CORE_LOG_FATAL_W_ERRNO(MAX_STRERROR_NUM);

@grom72 grom72 force-pushed the core_log_max branch 3 times, most recently from 71613f6 to 373ebcd Compare February 20, 2024 14:06
Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 12 files reviewed, 4 unresolved discussions (waiting on @janekmi)


src/core/log_internal.h line 114 at r1 (raw file):

 * If actual max message lenghth is eaqual to 408, it means we have a buffer
 * overflow
 */

Done.


src/core/log_internal.h line 115 at r1 (raw file):

 * overflow
 */
#define CORE_LOG_MSG_MAXPRINT 408 /* maximum expected log line */

Done.


src/test/core_log_max/call_all.c line 25 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I believe the #undef part is unnecessary.

  1. I don't think abort() is actually a definition in the first place but even if...
  2. ... you redefine it with the proceeding #define.

Done.


src/test/core_log_max/core_log_max.c line 179 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

You missed one.

Done.

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72)


src/test/core_log_max/core_log_max.c line 183 at r4 (raw file):

	UT_OUT("The_longest_message: %s", The_longest_message);
	UT_ASSERTeq(Max_message_len + 2, _CORE_LOG_MSG_MAXPRINT);

Suggestion:

/*
 * + 1 for '\n' and another
 * + 1 as a means for detecting too-long log messages. Please see _CORE_LOG_MSG_MAXPRINT for details.
 */
UT_ASSERTeq(Max_message_len + 2, _CORE_LOG_MSG_MAXPRINT);

Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion (waiting on @janekmi)


src/test/core_log_max/core_log_max.c line 183 at r4 (raw file):

	UT_OUT("The_longest_message: %s", The_longest_message);
	UT_ASSERTeq(Max_message_len + 2, _CORE_LOG_MSG_MAXPRINT);

Done.

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @grom72)

Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r6.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @grom72)

Signed-off-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com>
Copy link

codecov bot commented Feb 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (814dc95) 70.05% compared to head (8c1c62a) 70.06%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6016      +/-   ##
==========================================
+ Coverage   70.05%   70.06%   +0.01%     
==========================================
  Files         133      133              
  Lines       19570    19570              
  Branches     3266     3266              
==========================================
+ Hits        13710    13712       +2     
+ Misses       5860     5858       -2     

Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 12 files at r1, 2 of 5 files at r4, 4 of 4 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @grom72)

Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 5 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @grom72)

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 12 files at r1, 4 of 4 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @grom72)

@janekmi janekmi merged commit 8ff6a12 into pmem:master Feb 21, 2024
8 checks passed
@grom72 grom72 deleted the core_log_max branch February 21, 2024 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Add to skip the changelog check on your pull request sprint goal This pull request is part of the ongoing sprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants