| Message ID | 1585059422-84529-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Accepted |
| Commit | ecbc7f0aa2e112afc5ce63cf8a20ebd41e20b73b |
| Headers | show |
| Series | [ovs-dev,PATCHv2] fatal-signal: Fix clang error due to lock. | expand |
On 3/24/20 3:17 PM, William Tu wrote: > Due to not acquiring lock, clang reports: > lib/vlog.c:618:12: error: reading variable 'log_fd' requires holding mutex > 'log_file_mutex' [-Werror,-Wthread-safety-analysis] > return log_fd; > > The patch fixes it by creating a function in vlog.c to write > directly to log file unsafely. > > Tested-at: https://travis-ci.org/github/williamtu/ovs-travis/builds/666165883 > Fixes: ecd4a8fcdff2 ("fatal-signal: Log backtrace when no monitor daemon.") > Suggested-by: Ilya Maximets <i.maximets@ovn.org> > Signed-off-by: William Tu <u9012063@gmail.com> > --- > include/openvswitch/vlog.h | 4 ++-- > lib/fatal-signal.c | 8 ++------ > lib/vlog.c | 15 ++++++++++++--- > 3 files changed, 16 insertions(+), 11 deletions(-) > > diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h > index 476bf3d6d5b4..886fce5e0fd5 100644 > --- a/include/openvswitch/vlog.h > +++ b/include/openvswitch/vlog.h > @@ -143,8 +143,8 @@ void vlog_set_syslog_method(const char *method); > /* Configure syslog target. */ > void vlog_set_syslog_target(const char *target); > > -/* Return the log_fd. */ > -int vlog_get_fd(void); > +/* Write directly to log file. */ > +void vlog_direct_write_to_log_file_unsafe(const char *s); > > /* Initialization. */ > void vlog_init(void); > diff --git a/lib/fatal-signal.c b/lib/fatal-signal.c > index 4965c1ae82c0..51cf628d994e 100644 > --- a/lib/fatal-signal.c > +++ b/lib/fatal-signal.c > @@ -197,11 +197,7 @@ send_backtrace_to_monitor(void) { > */ > char str[] = "SIGSEGV detected, backtrace:\n"; > > - if (vlog_get_fd() < 0) { > - return; > - } > - > - ignore(write(vlog_get_fd(), str, strlen(str))); > + vlog_direct_write_to_log_file_unsafe(str); > > for (int i = 0; i < dep; i++) { > char line[64]; > @@ -210,7 +206,7 @@ send_backtrace_to_monitor(void) { > unw_bt[i].ip, > unw_bt[i].func, > unw_bt[i].offset); > - ignore(write(vlog_get_fd(), line, strlen(line))); > + vlog_direct_write_to_log_file_unsafe(line); > } > } > } > diff --git a/lib/vlog.c b/lib/vlog.c > index 502b33fc831e..6d17d4837e9c 100644 > --- a/lib/vlog.c > +++ b/lib/vlog.c > @@ -612,10 +612,19 @@ vlog_set_syslog_target(const char *target) > ovs_rwlock_unlock(&pattern_rwlock); > } > > -int > -vlog_get_fd(void) > +/* > + * This function writes directly to log file without using async writer or > + * taking a lock. Caller must hold 'log_file_mutex' or be sure that it's > + * not necessary. Could be used in exceptional cases like dumping of backtrace > + * on fatal signals. > + */ > +void > +vlog_direct_write_to_log_file_unsafe(const char *s) > + OVS_NO_THREAD_SAFETY_ANALYSIS > { > - return log_fd; > + if (log_fd >= 0) { > + ignore(write(log_fd, s, strlen(s))); > + } > } > > /* Returns 'false' if 'facility' is not a valid string. If 'facility' > So, does this work? I mean, last time you said that this exact version of code doesn't print anything. Best regards, Ilya Maximets.
On 3/24/20 3:27 PM, Ilya Maximets wrote: > On 3/24/20 3:17 PM, William Tu wrote: >> Due to not acquiring lock, clang reports: >> lib/vlog.c:618:12: error: reading variable 'log_fd' requires holding mutex >> 'log_file_mutex' [-Werror,-Wthread-safety-analysis] >> return log_fd; >> >> The patch fixes it by creating a function in vlog.c to write >> directly to log file unsafely. >> >> Tested-at: https://travis-ci.org/github/williamtu/ovs-travis/builds/666165883 >> Fixes: ecd4a8fcdff2 ("fatal-signal: Log backtrace when no monitor daemon.") >> Suggested-by: Ilya Maximets <i.maximets@ovn.org> >> Signed-off-by: William Tu <u9012063@gmail.com> >> --- I didn't test, but the code looks good to me. Acked-by: Ilya Maximets <i.maximets@ovn.org> >> include/openvswitch/vlog.h | 4 ++-- >> lib/fatal-signal.c | 8 ++------ >> lib/vlog.c | 15 ++++++++++++--- >> 3 files changed, 16 insertions(+), 11 deletions(-) >> >> diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h >> index 476bf3d6d5b4..886fce5e0fd5 100644 >> --- a/include/openvswitch/vlog.h >> +++ b/include/openvswitch/vlog.h >> @@ -143,8 +143,8 @@ void vlog_set_syslog_method(const char *method); >> /* Configure syslog target. */ >> void vlog_set_syslog_target(const char *target); >> >> -/* Return the log_fd. */ >> -int vlog_get_fd(void); >> +/* Write directly to log file. */ >> +void vlog_direct_write_to_log_file_unsafe(const char *s); >> >> /* Initialization. */ >> void vlog_init(void); >> diff --git a/lib/fatal-signal.c b/lib/fatal-signal.c >> index 4965c1ae82c0..51cf628d994e 100644 >> --- a/lib/fatal-signal.c >> +++ b/lib/fatal-signal.c >> @@ -197,11 +197,7 @@ send_backtrace_to_monitor(void) { >> */ >> char str[] = "SIGSEGV detected, backtrace:\n"; >> >> - if (vlog_get_fd() < 0) { >> - return; >> - } >> - >> - ignore(write(vlog_get_fd(), str, strlen(str))); >> + vlog_direct_write_to_log_file_unsafe(str); >> >> for (int i = 0; i < dep; i++) { >> char line[64]; >> @@ -210,7 +206,7 @@ send_backtrace_to_monitor(void) { >> unw_bt[i].ip, >> unw_bt[i].func, >> unw_bt[i].offset); >> - ignore(write(vlog_get_fd(), line, strlen(line))); >> + vlog_direct_write_to_log_file_unsafe(line); >> } >> } >> } >> diff --git a/lib/vlog.c b/lib/vlog.c >> index 502b33fc831e..6d17d4837e9c 100644 >> --- a/lib/vlog.c >> +++ b/lib/vlog.c >> @@ -612,10 +612,19 @@ vlog_set_syslog_target(const char *target) >> ovs_rwlock_unlock(&pattern_rwlock); >> } >> >> -int >> -vlog_get_fd(void) >> +/* >> + * This function writes directly to log file without using async writer or >> + * taking a lock. Caller must hold 'log_file_mutex' or be sure that it's >> + * not necessary. Could be used in exceptional cases like dumping of backtrace >> + * on fatal signals. >> + */ >> +void >> +vlog_direct_write_to_log_file_unsafe(const char *s) >> + OVS_NO_THREAD_SAFETY_ANALYSIS >> { >> - return log_fd; >> + if (log_fd >= 0) { >> + ignore(write(log_fd, s, strlen(s))); >> + } >> } >> >> /* Returns 'false' if 'facility' is not a valid string. If 'facility' >> > > So, does this work? > I mean, last time you said that this exact version of code doesn't print anything. Never mind. I saw the reply in a previous thread.
On Tue, Mar 24, 2020 at 03:38:31PM +0100, Ilya Maximets wrote: > On 3/24/20 3:27 PM, Ilya Maximets wrote: > > On 3/24/20 3:17 PM, William Tu wrote: > >> Due to not acquiring lock, clang reports: > >> lib/vlog.c:618:12: error: reading variable 'log_fd' requires holding mutex > >> 'log_file_mutex' [-Werror,-Wthread-safety-analysis] > >> return log_fd; > >> > >> The patch fixes it by creating a function in vlog.c to write > >> directly to log file unsafely. > >> > >> Tested-at: https://travis-ci.org/github/williamtu/ovs-travis/builds/666165883 > >> Fixes: ecd4a8fcdff2 ("fatal-signal: Log backtrace when no monitor daemon.") > >> Suggested-by: Ilya Maximets <i.maximets@ovn.org> > >> Signed-off-by: William Tu <u9012063@gmail.com> > >> --- > > I didn't test, but the code looks good to me. > > Acked-by: Ilya Maximets <i.maximets@ovn.org> Thanks, applied to master. William
diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h index 476bf3d6d5b4..886fce5e0fd5 100644 --- a/include/openvswitch/vlog.h +++ b/include/openvswitch/vlog.h @@ -143,8 +143,8 @@ void vlog_set_syslog_method(const char *method); /* Configure syslog target. */ void vlog_set_syslog_target(const char *target); -/* Return the log_fd. */ -int vlog_get_fd(void); +/* Write directly to log file. */ +void vlog_direct_write_to_log_file_unsafe(const char *s); /* Initialization. */ void vlog_init(void); diff --git a/lib/fatal-signal.c b/lib/fatal-signal.c index 4965c1ae82c0..51cf628d994e 100644 --- a/lib/fatal-signal.c +++ b/lib/fatal-signal.c @@ -197,11 +197,7 @@ send_backtrace_to_monitor(void) { */ char str[] = "SIGSEGV detected, backtrace:\n"; - if (vlog_get_fd() < 0) { - return; - } - - ignore(write(vlog_get_fd(), str, strlen(str))); + vlog_direct_write_to_log_file_unsafe(str); for (int i = 0; i < dep; i++) { char line[64]; @@ -210,7 +206,7 @@ send_backtrace_to_monitor(void) { unw_bt[i].ip, unw_bt[i].func, unw_bt[i].offset); - ignore(write(vlog_get_fd(), line, strlen(line))); + vlog_direct_write_to_log_file_unsafe(line); } } } diff --git a/lib/vlog.c b/lib/vlog.c index 502b33fc831e..6d17d4837e9c 100644 --- a/lib/vlog.c +++ b/lib/vlog.c @@ -612,10 +612,19 @@ vlog_set_syslog_target(const char *target) ovs_rwlock_unlock(&pattern_rwlock); } -int -vlog_get_fd(void) +/* + * This function writes directly to log file without using async writer or + * taking a lock. Caller must hold 'log_file_mutex' or be sure that it's + * not necessary. Could be used in exceptional cases like dumping of backtrace + * on fatal signals. + */ +void +vlog_direct_write_to_log_file_unsafe(const char *s) + OVS_NO_THREAD_SAFETY_ANALYSIS { - return log_fd; + if (log_fd >= 0) { + ignore(write(log_fd, s, strlen(s))); + } } /* Returns 'false' if 'facility' is not a valid string. If 'facility'
Due to not acquiring lock, clang reports: lib/vlog.c:618:12: error: reading variable 'log_fd' requires holding mutex 'log_file_mutex' [-Werror,-Wthread-safety-analysis] return log_fd; The patch fixes it by creating a function in vlog.c to write directly to log file unsafely. Tested-at: https://travis-ci.org/github/williamtu/ovs-travis/builds/666165883 Fixes: ecd4a8fcdff2 ("fatal-signal: Log backtrace when no monitor daemon.") Suggested-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: William Tu <u9012063@gmail.com> --- include/openvswitch/vlog.h | 4 ++-- lib/fatal-signal.c | 8 ++------ lib/vlog.c | 15 ++++++++++++--- 3 files changed, 16 insertions(+), 11 deletions(-)