On 3/24/20 3:30 PM, William Tu wrote: > On Mon, Mar 23, 2020 at 9:26 PM William Tu <[email protected]> wrote: >> >> On Mon, Mar 23, 2020 at 4:13 PM Ilya Maximets <[email protected]> wrote: >>> >>> On 3/23/20 11:34 PM, William Tu wrote: >>>> On Mon, Mar 23, 2020 at 2:58 PM William Tu <[email protected]> wrote: >>>>> >>>>> On Mon, Mar 23, 2020 at 2:34 PM Ben Pfaff <[email protected]> wrote: >>>>>> >>>>>> On Mon, Mar 23, 2020 at 02:22:17PM -0700, 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 by skipping it using OVS_NO_THREAD_SAFETY_ANALYSIS. >>>>>>> >>>>>>> Tested-at: >>>>>>> https://travis-ci.org/github/williamtu/ovs-travis/builds/666051210 >>>>>>> Fixex: ecd4a8fcdff2 ("fatal-signal: Log backtrace when no monitor >>>>>>> daemon.") >>>>>>> Signed-off-by: William Tu <[email protected]> >>>>>> >>>>>> You can't just mark the existing send_backtrace_to_monitor() >>>>>> OVS_NO_THREAD_SAFETY_ANALYSIS? It seems like the easiest change. >>>>> OK >>>>> that will also work. >>>>>> >>>>>> Also, the OVS_REQUIRES should also go in vlog.h. >>>> >>>> Adding 'OVS_REQUIRES(log_file_mutext)' to vlog.h has error like this: >>>> ./include/openvswitch/vlog.h:147:36: error: use of undeclared >>>> identifier 'log_file_mutex' >>>> int vlog_get_fd(void) OVS_REQUIRES(log_file_mutex); >>>> >>>> I will have to move log_file_mutex and pattern_rwlock to vlog.h. >>>> Do we want this? >>>> Does it make difference if OVS_REQUIRES is only used in C file >>>> but not in header file? >>> >>> To avoid exposure of the vlog internals, maybe we could create a new >>> function like: >>> >>> /* >>> * 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 >>> { >>> if (log_fd >= 0) { >>> ignore(write(log_fd, s, strlen(s))); >>> } >>> } >>> >> >> Hi Ilya, >> >> Do you know any restrictions when calling functions in signal handler? >> When use this approach, the backtrace does not print anything. >> I'm still debugging it with the below patch: >> >> 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' > > I have the issue fixed and sent v2 patch > https://mail.openvswitch.org/pipermail/ovs-dev/2020-March/369070.html > > The issue was that I compile the code with address sanitizer below: > ./configure CC=clang --enable-Werror CFLAGS="-g -O2 -fsanitize=address" > Then no backtrace is shown. > Remove '-fsanitize=address' or use GCC works OK.
OK. Makes sense. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
