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. Thanks William _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
