[ovs-dev] [PATCH] fatal-signal: Skip acquiring log fd lock.

Ben Pfaff blp at ovn.org
Mon Mar 23 23:49:35 UTC 2020


On Tue, Mar 24, 2020 at 12:13:12AM +0100, Ilya Maximets wrote:
> On 3/23/20 11:34 PM, William Tu wrote:
> > On Mon, Mar 23, 2020 at 2:58 PM William Tu <u9012063 at gmail.com> wrote:
> >>
> >> On Mon, Mar 23, 2020 at 2:34 PM Ben Pfaff <blp at ovn.org> 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 <u9012063 at gmail.com>
> >>>
> >>> 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)));
>     }
> }
> 
> William, Ben, WDYT?

I like that solution.  Thanks.


More information about the dev mailing list