[ovs-dev,PATCHv2] fatal-signal: Fix clang error due to lock.
diff mbox series

Message ID 1585059422-84529-1-git-send-email-u9012063@gmail.com
State New
Headers show
Series
  • [ovs-dev,PATCHv2] fatal-signal: Fix clang error due to lock.
Related show

Commit Message

William Tu March 24, 2020, 2:17 p.m. UTC
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(-)

Comments

Ilya Maximets March 24, 2020, 2:27 p.m. UTC | #1
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.
Ilya Maximets March 24, 2020, 2:38 p.m. UTC | #2
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.
William Tu March 24, 2020, 2:48 p.m. UTC | #3
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

Patch
diff mbox series

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'