[ovs-dev] [PATCHv2] fatal-signal: Fix clang error due to lock.
Ilya Maximets
i.maximets at ovn.org
Tue Mar 24 14:38:31 UTC 2020
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 at ovn.org>
>> Signed-off-by: William Tu <u9012063 at gmail.com>
>> ---
I didn't test, but the code looks good to me.
Acked-by: Ilya Maximets <i.maximets at 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.
More information about the dev
mailing list