-
Notifications
You must be signed in to change notification settings - Fork 510
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
Replace ERR("!... ") with ERR_W_ERRNO #5965
Conversation
a154b4c
to
e41822e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1, 7 of 7 files at r2, all commit messages.
Reviewable status: 10 of 56 files reviewed, 20 unresolved discussions (waiting on @grom72)
src/core/log_internal.h
line 146 at r3 (raw file):
/* * Replacement for ERR("!*") macro (w/ errno).
This comment won't stand long. At some point there will be no ERR("!...
any more.
src/core/log_internal.h
line 154 at r3 (raw file):
CORE_LOG_ERROR(f " failed: %s", ##__VA_ARGS__,\ strerror(errno));\ } while (0)
- It is a single command. No need to wrap it with
do-while
. - I like having a space before each of the
\
at the end of the line. And it is consistent with the rest of the file so far.
Suggestion:
#define CORE_LOG_ERROR_WITH_ERRNO(f, ...) \
CORE_LOG_ERROR(f " failed: %s", ##__VA_ARGS__, strerror(errno))
src/core/out.h
line 192 at r3 (raw file):
#define ERR_W_ERRNO(f, ...)\ do {\ ERR("*!" f, ##__VA_ARGS__);\
Suggestion:
ERR("!" f, ##__VA_ARGS__);\
src/core/out.h
line 197 at r3 (raw file):
#define ERR_WO_ERRNO(f, ...)\ do { \
Not a fan. Just for consistency.
Suggestion:
do {\
src/core/out.c
line 393 at r3 (raw file):
print_msg = 0; fmt++; }
Is it a real problem you stumbled upon?
Code quote:
/*
* '*' at the begining means 'do not push message to output'
*/
if (*fmt == '*') {
print_msg = 0;
fmt++;
}
src/test/log_errno/.gitignore
line 5 at r3 (raw file):
*.log logs .deps
Do you really need all these definitions? It seems other .gitignore
files do not have them.
Suggestion:
log_errno
src/test/log_errno/log_errno.c
line 5 at r3 (raw file):
/* * traces.c -- unit test for traces
Do you plan to expand this one to accommodate other macros as well?
Maybe the log_errno
name is actually too limiting?
Suggestion:
* log_errno.c -- unit test for the CORE_LOG_ERROR_WITH_ERRNO macro
src/test/log_errno/log_errno.c
line 14 at r3 (raw file):
#define MAJOR_VERSION 1 #define MINOR_VERSION 0 #endif
src/test/log_errno/log_errno.c
line 17 at r3 (raw file):
#include <sys/types.h> #include <stdarg.h>
Are you sure you need these two?
Code quote:
#include <sys/types.h>
#include <stdarg.h>
src/test/log_errno/log_errno.c
line 24 at r3 (raw file):
main(int argc, char *argv[]) { // char buff[UT_MAX_ERR_MSG];
src/test/log_errno/Makefile
line 2 at r3 (raw file):
# SPDX-License-Identifier: BSD-3-Clause # Copyright 2015-2016, Intel Corporation
Suggestion:
2024
src/test/log_errno/Makefile
line 5 at r3 (raw file):
# # src/test/log_errno/Makefile -- build unit test for log_errno()
Please fix.
Code quote:
build unit test for log_errno()
src/test/log_errno/out0.log.match
line 3 at r3 (raw file):
log_errno$(nW)TEST0: START: log_errno$(nW) $(nW)log_errno$(nW) log_errno$(nW)TEST0: DONE
It looks unnecessary.
src/test/log_errno/TEST0
line 1 at r3 (raw file):
#!/usr/bin/env bash
Have you decided to add yet another Bash unit test?
src/test/log_errno/TEST0
line 3 at r3 (raw file):
#!/usr/bin/env bash # SPDX-License-Identifier: BSD-3-Clause # Copyright 2015-2019, Intel Corporation
Suggestion:
2024
src/test/log_errno/TEST0
line 6 at r3 (raw file):
# # src/test/out_err/TEST0 -- unit test for out_err()
Please fix.
Code quote:
# src/test/out_err/TEST0 -- unit test for out_err()
src/test/log_errno/TEST0
line 11 at r3 (raw file):
. ../unittest/unittest.sh require_test_type medium
Suggestion:
require_test_type short
src/test/log_errno/TEST0
line 14 at r3 (raw file):
require_fs_type none require_build_type debug
Do you need both this requirement and CFLAGS += -DDEBUG
in the Makefile?
Code quote:
require_build_type debug
src/test/log_errno/TEST0
line 20 at r3 (raw file):
export TRACE_LOG_LEVEL=1 export TRACE_LOG_FILE=./traces$UNITTEST_NUM.log export UNITTEST_DO_NOT_CHECK_OPEN_FILES=1
I believe all of these are redundant, is it correct?
src/test/log_errno/TEST0
line 22 at r3 (raw file):
export UNITTEST_DO_NOT_CHECK_OPEN_FILES=1 expect_normal_exit ./log_errno$EXESUFFIX 2>err0.log
I believe you do not need this bit.
Suggestion:
expect_normal_exit ./log_errno$EXESUFFIX
e41822e
to
5af3bc4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 56 files reviewed, 20 unresolved discussions (waiting on @janekmi)
src/core/log_internal.h
line 146 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
This comment won't stand long. At some point there will be no
ERR("!...
any more.
Let's keep it until we know, we can remove ERR(
macro
src/core/log_internal.h
line 154 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
- It is a single command. No need to wrap it with
do-while
.- I like having a space before each of the
\
at the end of the line. And it is consistent with the rest of the file so far.
Done.
src/core/out.h
line 192 at r3 (raw file):
#define ERR_W_ERRNO(f, ...)\ do {\ ERR("*!" f, ##__VA_ARGS__);\
No, *
prevents double error messages on the output.
src/core/out.h
line 197 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Not a fan. Just for consistency.
Done.
src/core/out.c
line 393 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Is it a real problem you stumbled upon?
Test logs are corrupted without this change..
src/test/log_errno/.gitignore
line 5 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Do you really need all these definitions? It seems other
.gitignore
files do not have them.
I know, but it does not mean others are proper ones.
src/test/log_errno/log_errno.c
line 5 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Do you plan to expand this one to accommodate other macros as well?
Maybe thelog_errno
name is actually too limiting?
Done.
src/test/log_errno/log_errno.c
line 14 at r3 (raw file):
#define MAJOR_VERSION 1 #define MINOR_VERSION 0 #endif
Done.
src/test/log_errno/log_errno.c
line 17 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Are you sure you need these two?
Done.
src/test/log_errno/log_errno.c
line 24 at r3 (raw file):
main(int argc, char *argv[]) { // char buff[UT_MAX_ERR_MSG];
Done.
src/test/log_errno/Makefile
line 2 at r3 (raw file):
# SPDX-License-Identifier: BSD-3-Clause # Copyright 2015-2016, Intel Corporation
Done.
src/test/log_errno/Makefile
line 5 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Please fix.
Done.
src/test/log_errno/TEST0
line 1 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Have you decided to add yet another Bash unit test?
Yes, I like it
src/test/log_errno/TEST0
line 3 at r3 (raw file):
#!/usr/bin/env bash # SPDX-License-Identifier: BSD-3-Clause # Copyright 2015-2019, Intel Corporation
Done.
src/test/log_errno/TEST0
line 6 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Please fix.
Done.
src/test/log_errno/TEST0
line 11 at r3 (raw file):
. ../unittest/unittest.sh require_test_type medium
Done.
src/test/log_errno/TEST0
line 14 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Do you need both this requirement and
CFLAGS += -DDEBUG
in the Makefile?
Done.
src/test/log_errno/TEST0
line 20 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
I believe all of these are redundant, is it correct?
Done.
src/test/log_errno/TEST0
line 22 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
I believe you do not need this bit.
We test stderr only, so redirection is required. Otherwise we have mixture of test framework messages and error log output
src/test/log_errno/out0.log.match
line 3 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
It looks unnecessary.
It is a scaffolding
5af3bc4
to
95cccad
Compare
4ec381e
to
c52e35e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 55 files at r4, 4 of 7 files at r5, 1 of 47 files at r6, 2 of 48 files at r7, 24 of 47 files at r8, 4 of 4 files at r10, 23 of 23 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @grom72)
a discussion (no related file):
You are about to squash the commits in the end?
src/common/set.c
line 370 at r12 (raw file):
ERR_W_ERRNO("unlink %s failed (part %u,"\ " replica %u)",\ rep->part[p].path, p, repn);
Breaking error message strings is a bad idea.
Note you can have a as long string as you want if you start from the beginning of the line.
Suggestion:
ERR_W_ERRNO(
"unlink %s failed (part %u, replica %u)",
rep->part[p].path, p, repn);
src/common/set.c
line 421 at r12 (raw file):
os_stat_t stbuf; if (os_fstat(part->fd, &stbuf) != 0) { ERR_W_ERRNO("fstat %d %s", part->fd,\
Suggestion:
ERR_W_ERRNO("fstat %d %s", part->fd,
src/common/set.c
line 1497 at r12 (raw file):
if (ret != 0) { errno = ret; ERR_W_ERRNO("posix_fallocate \"%s\", %zu",\
Suggestion:
ERR_W_ERRNO("posix_fallocate \"%s\", %zu",
src/core/out.h
line 193 at r12 (raw file):
do {\ ERR("*!" f, ##__VA_ARGS__);\ } while (0)
It looks like no core logging is used. What is the point then?
Code quote:
#define ERR_W_ERRNO(f, ...)\
do {\
ERR("*!" f, ##__VA_ARGS__);\
} while (0)
src/libpmempool/replica.c
line 191 at r12 (raw file):
if (os_unlink(phs->recovery_file_name) < 0) { ERR_W_ERRNO("removing the bad block recovery file failed"\ " -- '%s'", phs->recovery_file_name);
Suggestion:
ERR_W_ERRNO(
"removing the bad block recovery file failed -- '%s'",
phs->recovery_file_name);
src/libpmempool/replica.c
line 759 at r12 (raw file):
if (!recovery_file) { ERR_W_ERRNO("opening the recovery file for reading failed "\ "-- '%s'", path);
Suggestion:
ERR_W_ERRNO(
"opening the recovery file for reading failed -- '%s'",
path);
src/test/log_errno/.gitignore
line 5 at r3 (raw file):
Previously, grom72 (Tomasz Gromadzki) wrote…
I know, but it does not mean others are proper ones.
Just tested. All of them - redundant.
src/test/log_errno/log_errno.c
line 19 at r12 (raw file):
CORE_LOG_ERROR_WITH_ERRNO("open file %s", "lolek"); DONE(NULL); }
Suggestion:
#include <syslog.h>
#include "unittest.h"
#include "log_internal.h"
int
main(int argc, char *argv[])
{
START(argc, argv, "log_errno");
core_log_init();
CORE_LOG_ERROR_WITH_ERRNO("open file %s", "lolek");
core_log_fini();
/*
* The fini function above intentionally does not close the syslog
* socket. It has to be closed separately so it won't be accounted as
* an unclosed file descriptor.
*/
closelog();
DONE(NULL);
}
src/test/log_errno/Makefile
line 5 at r12 (raw file):
# # src/test/log_errno/Makefile -- build unit test for log_errno()
Suggestion:
build unit test for log_errno
src/test/log_errno/TEST0
line 16 at r12 (raw file):
setup export UNITTEST_DO_NOT_CHECK_OPEN_FILES=1
Please see other comments for more details.
src/test/log_errno/TEST0
line 18 at r12 (raw file):
export UNITTEST_DO_NOT_CHECK_OPEN_FILES=1 expect_normal_exit ./log_errno$EXESUFFIX 2>err0.log
Suggestion:
expect_normal_exit ./log_errno$EXESUFFIX 2>$ERR_LOG_FILE
src/test/pmempool_sync/TEST10
line 56 at r12 (raw file):
# Suppress error messages produced by CORE_LOG_ERROR() fgrep -v "*ERROR*" $LOG_TEMP > $LOG rm $LOG_TEMP
.
Code quote:
# Suppress error messages produced by CORE_LOG_ERROR()
fgrep -v "*ERROR*" $LOG_TEMP > $LOG
rm $LOG_TEMP
src/test/pmempool_sync/TEST23
line 68 at r12 (raw file):
# Suppress error messages produced by CORE_LOG_ERROR() fgrep -v "*ERROR*" $LOG_TEMP > $LOG rm $LOG_TEMP
.
Code quote:
# Suppress error messages produced by CORE_LOG_ERROR()
fgrep -v "*ERROR*" $LOG_TEMP > $LOG
rm $LOG_TEMP
src/test/pmempool_sync/TEST24
line 65 at r12 (raw file):
# Suppress error messages produced by CORE_LOG_ERROR() fgrep -v "*ERROR*" $LOG_TEMP > $LOG rm $LOG_TEMP
.
Code quote:
# Suppress error messages produced by CORE_LOG_ERROR()
fgrep -v "*ERROR*" $LOG_TEMP > $LOG
rm $LOG_TEMP
src/test/pmempool_sync/TEST9
line 63 at r12 (raw file):
# Suppress error messages produced by CORE_LOG_ERROR() fgrep -v "*ERROR*" $LOG_TEMP > $LOG rm $LOG_TEMP
It is unnecessary at this point since the CORE_LOG_ERROR is not actually in use.
Code quote:
# Suppress error messages produced by CORE_LOG_ERROR()
fgrep -v "*ERROR*" $LOG_TEMP > $LOG
rm $LOG_TEMP
b1f1054
to
06d338b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 76 files reviewed, 16 unresolved discussions (waiting on @janekmi)
a discussion (no related file):
Previously, janekmi (Jan Michalski) wrote…
You are about to squash the commits in the end?
Yes as soon as all tests will pass.
src/common/set.c
line 370 at r12 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Breaking error message strings is a bad idea.
Note you can have a as long string as you want if you start from the beginning of the line.
Done.
src/common/set.c
line 421 at r12 (raw file):
os_stat_t stbuf; if (os_fstat(part->fd, &stbuf) != 0) { ERR_W_ERRNO("fstat %d %s", part->fd,\
Done.
src/common/set.c
line 1497 at r12 (raw file):
if (ret != 0) { errno = ret; ERR_W_ERRNO("posix_fallocate \"%s\", %zu",\
Done.
src/core/out.h
line 193 at r12 (raw file):
Previously, janekmi (Jan Michalski) wrote…
It looks like no core logging is used. What is the point then?
It seems that I made some mistakes while rebasing.
src/libpmempool/replica.c
line 191 at r12 (raw file):
if (os_unlink(phs->recovery_file_name) < 0) { ERR_W_ERRNO("removing the bad block recovery file failed"\ " -- '%s'", phs->recovery_file_name);
Done.
src/libpmempool/replica.c
line 759 at r12 (raw file):
if (!recovery_file) { ERR_W_ERRNO("opening the recovery file for reading failed "\ "-- '%s'", path);
Done.
src/test/log_errno/.gitignore
line 5 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Just tested. All of them - redundant.
Done.
src/test/log_errno/log_errno.c
line 19 at r12 (raw file):
CORE_LOG_ERROR_WITH_ERRNO("open file %s", "lolek"); DONE(NULL); }
Done.
src/test/log_errno/Makefile
line 5 at r12 (raw file):
# # src/test/log_errno/Makefile -- build unit test for log_errno()
Done.
src/test/log_errno/TEST0
line 16 at r12 (raw file):
Previously, janekmi (Jan Michalski) wrote…
Please see other comments for more details.
Done.
src/test/log_errno/TEST0
line 18 at r12 (raw file):
export UNITTEST_DO_NOT_CHECK_OPEN_FILES=1 expect_normal_exit ./log_errno$EXESUFFIX 2>err0.log
Done.
src/test/pmempool_sync/TEST10
line 56 at r12 (raw file):
Previously, janekmi (Jan Michalski) wrote…
.
.
src/test/pmempool_sync/TEST23
line 68 at r12 (raw file):
Previously, janekmi (Jan Michalski) wrote…
.
.
src/test/pmempool_sync/TEST24
line 65 at r12 (raw file):
Previously, janekmi (Jan Michalski) wrote…
.
.
src/test/pmempool_sync/TEST9
line 63 at r12 (raw file):
Previously, janekmi (Jan Michalski) wrote…
It is unnecessary at this point since the CORE_LOG_ERROR is not actually in use.
CORE_LOG_ERROR
is back in use.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #5965 +/- ##
==========================================
+ Coverage 68.47% 68.52% +0.04%
==========================================
Files 133 133
Lines 19660 19664 +4
Branches 3261 3262 +1
==========================================
+ Hits 13463 13474 +11
+ Misses 6197 6190 -7 |
10648d2
to
636de1d
Compare
cda15e8
to
c231dd8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 55 of 55 files at r15, 19 of 19 files at r16, 4 of 4 files at r17, 1 of 1 files at r18, 1 of 1 files at r19, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @grom72)
src/test/pmempool_sync/TEST10
line 54 at r19 (raw file):
dump_pool_info $POOLSET1 >> $LOG_TEMP # Suppress error messages produced by CORE_LOG_ERROR()
Suggestion:
# Exclude error messages printed out on the stderr by PMDK in debug
src/test/pmempool_sync/TEST23
line 66 at r19 (raw file):
dump_pool_info $POOLSET1 >> $LOG_TEMP # Suppress error messages produced by CORE_LOG_ERROR()
Suggestion:
# Exclude error messages printed out on the stderr by PMDK in debug
src/test/pmempool_sync/TEST24
line 63 at r19 (raw file):
dump_pool_info $POOLSET1 >> $LOG_TEMP # Suppress error messages produced by CORE_LOG_ERROR()
Suggestion:
# Exclude error messages printed out on the stderr by PMDK in debug
src/test/pmempool_sync/TEST9
line 61 at r19 (raw file):
expect_abnormal_exit $PMEMPOOL$EXESUFFIX sync $POOLSET2 >> $LOG_TEMP 2>&1 # Suppress error messages produced by CORE_LOG_ERROR()
Suggestion:
# Exclude error messages printed out on the stderr by PMDK in debug
ERR() macro uses CORE_LOG_ERROR to push mesages to ouputs Introduce to separate macros to report errors w/ and w/o errno. test for CORE_LOG_ERROR_WITH_ERRNO Signed-off-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com>
Signed-off-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com>
c231dd8
to
fa78192
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 7 of 76 files at r13, 45 of 55 files at r15.
Reviewable status: 75 of 81 files reviewed, 4 unresolved discussions (waiting on @janekmi)
src/core/out.h
line 193 at r12 (raw file):
Previously, grom72 (Tomasz Gromadzki) wrote…
It seems that I made some mistakes while rebasing.
Done
src/test/pmempool_sync/TEST10
line 54 at r19 (raw file):
dump_pool_info $POOLSET1 >> $LOG_TEMP # Suppress error messages produced by CORE_LOG_ERROR()
Done.
src/test/pmempool_sync/TEST23
line 66 at r19 (raw file):
dump_pool_info $POOLSET1 >> $LOG_TEMP # Suppress error messages produced by CORE_LOG_ERROR()
Done.
src/test/pmempool_sync/TEST24
line 63 at r19 (raw file):
dump_pool_info $POOLSET1 >> $LOG_TEMP # Suppress error messages produced by CORE_LOG_ERROR()
Done.
src/test/pmempool_sync/TEST9
line 61 at r19 (raw file):
expect_abnormal_exit $PMEMPOOL$EXESUFFIX sync $POOLSET2 >> $LOG_TEMP 2>&1 # Suppress error messages produced by CORE_LOG_ERROR()
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r21, 1 of 1 files at r22, 1 of 1 files at r23, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @grom72)
Replace
ERR("!... ")
withERR_W_ERRNO
Requires:
This change is