sox-devel@lists.sourceforge.net unofficial mirror
 help / color / mirror / code / Atom feed
From: "Md. Enzam Hossain via SoX-devel" <sox-devel@lists.sourceforge.net>
To: sox-devel@lists.sourceforge.net
Cc: "Md. Enzam Hossain" <enzam@google.com>
Subject: Patch to make logging thread-frinedly
Date: Wed, 8 Sep 2021 13:23:22 -0700	[thread overview]
Message-ID: <CAO9x7dKWc=_12-2mykAqMV8xFd_EMSVi6S6G3Pfu3GyAM1cdkQ@mail.gmail.com> (raw)


[-- Attachment #1.1: Type: text/plain, Size: 8673 bytes --]

The logging functions in SoX like lsx_debug, lsx_report, etc. write to the
global struct sox_globals before calling the actual print functions.

Example macro:
#define lsx_report sox_globals.subsystem=__FILE__,lsx_report_impl

This can create *race conditions* if run parallely.

For example, two lsx_report calls are called parallely.
lsx_report call 1 updates sox_globals.subsystem.
lsx_report call 2 updates sox_globals.subsystem.
lsx_report call 1 prints, but with incorrect subsystem of call 2.
lsx_report call 2 prints with the correct subsystem.

I am attaching a patch file to make the calls thread-friendly.
Please let me know if the patch makes sense.

--

Regards,
Enzam (enzam@ <http://who/enzam>)

*Inlined patch (also attached as file):*

From 3be8226c8ca2b4f29b6927583641cb08ef39dddf Mon Sep 17 00:00:00 2001
From: "Md. Enzam Hossain" <enzam@google.com>
Date: Wed, 8 Sep 2021 12:03:02 -0700
Subject: [PATCH] Make logging thread friendly.

If multiple threads tries to log together, the subsystem may get
overwritten by each other.
Remove the dependency on the global parameter.
---
 src/libsox.c |  4 ++--
 src/mp3.c    | 15 +++------------
 src/sox.h    | 32 ++++++++++++++++++--------------
 src/sox_i.h  | 16 ++++++++--------
 4 files changed, 31 insertions(+), 36 deletions(-)

diff --git a/src/libsox.c b/src/libsox.c
index 8e9ebad6..6fc0ec41 100644
--- a/src/libsox.c
+++ b/src/libsox.c
@@ -195,11 +195,11 @@ size_t sox_basename(char * base_buffer, size_t
base_buffer_len, const char * fil
 }

 #define SOX_MESSAGE_FUNCTION(name,level) \
-void name(char const * fmt, ...) { \
+void name(char const * filename, char const * fmt, ...) { \
   va_list ap; \
   va_start(ap, fmt); \
   if (sox_globals.output_message_handler) \
-
 (*sox_globals.output_message_handler)(level,sox_globals.subsystem,fmt,ap);
\
+    (*sox_globals.output_message_handler)(level,filename,fmt,ap); \
   va_end(ap); \
 }

diff --git a/src/mp3.c b/src/mp3.c
index 1fd144d1..552ab0db 100644
--- a/src/mp3.c
+++ b/src/mp3.c
@@ -680,26 +680,17 @@ static int startread(sox_format_t * ft)

 static void errorf(const char* fmt, va_list va)
 {
-  sox_globals.subsystem=__FILE__;
-  if (sox_globals.output_message_handler)
-    (*sox_globals.output_message_handler)(1,sox_globals.subsystem,fmt,va);
-  return;
+  lsx_fail(fmt, va);
 }

 static void debugf(const char* fmt, va_list va)
 {
-  sox_globals.subsystem=__FILE__;
-  if (sox_globals.output_message_handler)
-    (*sox_globals.output_message_handler)(4,sox_globals.subsystem,fmt,va);
-  return;
+  lsx_debug(fmt, va);
 }

 static void msgf(const char* fmt, va_list va)
 {
-  sox_globals.subsystem=__FILE__;
-  if (sox_globals.output_message_handler)
-    (*sox_globals.output_message_handler)(3,sox_globals.subsystem,fmt,va);
-  return;
+  lsx_report(fmt, va);
 }

 /* These functions are considered optional. If they aren't present in the
diff --git a/src/sox.h b/src/sox.h
index bac50354..b9cd9754 100644
--- a/src/sox.h
+++ b/src/sox.h
@@ -79,14 +79,14 @@ the variable being unused (especially in
macro-generated code).

 /**
 Plugins API:
-LSX_PRINTF12: Attribute applied to a function to indicate that it requires
-a printf-style format string for arg1 and that printf parameters start at
-arg2.
+LSX_PRINTF23: Attribute applied to a function to indicate that it requires
+a printf-style format string for arg2 and that printf parameters start at
+arg3.
 */
 #ifdef __GNUC__
-#define LSX_PRINTF12  __attribute__ ((format (printf, 1, 2))) /* Function
has printf-style arguments. */
+#define LSX_PRINTF23  __attribute__ ((format (printf, 2, 3))) /* Function
has printf-style arguments. */
 #else
-#define LSX_PRINTF12 /* Function has printf-style arguments. */
+#define LSX_PRINTF23 /* Function has printf-style arguments. */
 #endif

 /**
@@ -1330,7 +1330,7 @@ typedef struct sox_globals_t {

   char const * stdin_in_use_by;  /**< Private: tracks the name of the
handler currently using stdin */
   char const * stdout_in_use_by; /**< Private: tracks the name of the
handler currently using stdout */
-  char const * subsystem;        /**< Private: tracks the name of the
handler currently writing an output message */
+  char const * subsystem;        /**< Deprecated, Private: tracks the name
of the handler currently writing an output message */
   char       * tmp_path;         /**< Private: client-configured path to
use for temporary files */
   sox_bool     use_magic;        /**< Private: true if client has
requested use of 'magic' file-type detection */
   sox_bool     use_threads;      /**< Private: true if client has
requested parallel effects processing */
@@ -2273,9 +2273,10 @@ Print a fatal error in libSoX.
 void
 LSX_API
 lsx_fail_impl(
+    LSX_PARAM_IN_Z char const * filename, /**< Source code __FILE__ from
which message originates. */
     LSX_PARAM_IN_PRINTF char const * fmt, /**< printf-style format string.
*/
     ...)
-    LSX_PRINTF12;
+    LSX_PRINTF23;

 /**
 Plugins API:
@@ -2284,9 +2285,10 @@ Print a warning in libSoX.
 void
 LSX_API
 lsx_warn_impl(
+    LSX_PARAM_IN_Z char const * filename, /**< Source code __FILE__ from
which message originates. */
     LSX_PARAM_IN_PRINTF char const * fmt, /**< printf-style format string.
*/
     ...)
-    LSX_PRINTF12;
+    LSX_PRINTF23;

 /**
 Plugins API:
@@ -2295,9 +2297,10 @@ Print an informational message in libSoX.
 void
 LSX_API
 lsx_report_impl(
+    LSX_PARAM_IN_Z char const * filename, /**< Source code __FILE__ from
which message originates. */
     LSX_PARAM_IN_PRINTF char const * fmt, /**< printf-style format string.
*/
     ...)
-    LSX_PRINTF12;
+    LSX_PRINTF23;

 /**
 Plugins API:
@@ -2306,33 +2309,34 @@ Print a debug message in libSoX.
 void
 LSX_API
 lsx_debug_impl(
+    LSX_PARAM_IN_Z char const * filename, /**< Source code __FILE__ from
which message originates. */
     LSX_PARAM_IN_PRINTF char const * fmt, /**< printf-style format string.
*/
     ...)
-    LSX_PRINTF12;
+    LSX_PRINTF23;

 /**
 Plugins API:
 Report a fatal error in libSoX; printf-style arguments must follow.
 */
-#define lsx_fail       sox_get_globals()->subsystem=__FILE__,lsx_fail_impl
+#define lsx_fail(...)       lsx_fail_impl(__FILE__, __VA_ARGS__)

 /**
 Plugins API:
 Report a warning in libSoX; printf-style arguments must follow.
 */
-#define lsx_warn       sox_get_globals()->subsystem=__FILE__,lsx_warn_impl
+#define lsx_warn(...)       lsx_warn_impl(__FILE__, __VA_ARGS__)

 /**
 Plugins API:
 Report an informational message in libSoX; printf-style arguments must
follow.
 */
-#define lsx_report
sox_get_globals()->subsystem=__FILE__,lsx_report_impl
+#define lsx_report(...)     lsx_report_impl(__FILE__, __VA_ARGS__)

 /**
 Plugins API:
 Report a debug message in libSoX; printf-style arguments must follow.
 */
-#define lsx_debug      sox_get_globals()->subsystem=__FILE__,lsx_debug_impl
+#define lsx_debug(...)      lsx_debug_impl(__FILE__, __VA_ARGS__)

 /**
 Plugins API:
diff --git a/src/sox_i.h b/src/sox_i.h
index c8552f97..c843556c 100644
--- a/src/sox_i.h
+++ b/src/sox_i.h
@@ -31,10 +31,10 @@
 #undef lsx_fail
 #undef lsx_report
 #undef lsx_warn
-#define lsx_debug sox_globals.subsystem=effp->handler.name,lsx_debug_impl
-#define lsx_fail sox_globals.subsystem=effp->handler.name,lsx_fail_impl
-#define lsx_report sox_globals.subsystem=effp->handler.name,lsx_report_impl
-#define lsx_warn sox_globals.subsystem=effp->handler.name,lsx_warn_impl
+#define lsx_debug(...) lsx_debug_impl(effp->handler.name, __VA_ARGS__)
+#define lsx_fail(...) lsx_fail_impl(effp->handler.name, __VA_ARGS__)
+#define lsx_report(...) lsx_report_impl(effp->handler.name, __VA_ARGS__)
+#define lsx_warn(...) lsx_warn_impl(effp->handler.name, __VA_ARGS__)
 #endif

 #define RANQD1 ranqd1(sox_globals.ranqd1)
@@ -56,11 +56,11 @@ assert_static(sizeof(off_t) == _FILE_OFFSET_BITS >> 3,
OFF_T_BUILD_PROBLEM);

 FILE * lsx_tmpfile(void);

-void lsx_debug_more_impl(char const * fmt, ...) LSX_PRINTF12;
-void lsx_debug_most_impl(char const * fmt, ...) LSX_PRINTF12;
+void lsx_debug_more_impl(LSX_PARAM_IN_Z char const * filename,
LSX_PARAM_IN_PRINTF char const * fmt, ...) LSX_PRINTF23;
+void lsx_debug_most_impl(LSX_PARAM_IN_Z char const * filename,
LSX_PARAM_IN_PRINTF char const * fmt, ...) LSX_PRINTF23;

-#define lsx_debug_more
sox_get_globals()->subsystem=__FILE__,lsx_debug_more_impl
-#define lsx_debug_most
sox_get_globals()->subsystem=__FILE__,lsx_debug_most_impl
+#define lsx_debug_more(...) lsx_debug_more_impl(__FILE__, __VA_ARGS__)
+#define lsx_debug_most(...) lsx_debug_most_impl(__FILE__, __VA_ARGS__)

 /* Digitise one cycle of a wave and store it as
  * a table of samples of a specified data-type.
-- 
2.33.0.153.gba50c8fa24-goog

[-- Attachment #1.2: Type: text/html, Size: 10584 bytes --]

[-- Attachment #2: 0001-Make-logging-thread-friendly.patch --]
[-- Type: text/x-patch, Size: 7949 bytes --]

From 3be8226c8ca2b4f29b6927583641cb08ef39dddf Mon Sep 17 00:00:00 2001
From: "Md. Enzam Hossain" <enzam@google.com>
Date: Wed, 8 Sep 2021 12:03:02 -0700
Subject: [PATCH] Make logging thread friendly.

If multiple threads tries to log together, the subsystem may get
overwritten by each other.
Remove the dependency on the global parameter.
---
 src/libsox.c |  4 ++--
 src/mp3.c    | 15 +++------------
 src/sox.h    | 32 ++++++++++++++++++--------------
 src/sox_i.h  | 16 ++++++++--------
 4 files changed, 31 insertions(+), 36 deletions(-)

diff --git a/src/libsox.c b/src/libsox.c
index 8e9ebad6..6fc0ec41 100644
--- a/src/libsox.c
+++ b/src/libsox.c
@@ -195,11 +195,11 @@ size_t sox_basename(char * base_buffer, size_t base_buffer_len, const char * fil
 }
 
 #define SOX_MESSAGE_FUNCTION(name,level) \
-void name(char const * fmt, ...) { \
+void name(char const * filename, char const * fmt, ...) { \
   va_list ap; \
   va_start(ap, fmt); \
   if (sox_globals.output_message_handler) \
-    (*sox_globals.output_message_handler)(level,sox_globals.subsystem,fmt,ap); \
+    (*sox_globals.output_message_handler)(level,filename,fmt,ap); \
   va_end(ap); \
 }
 
diff --git a/src/mp3.c b/src/mp3.c
index 1fd144d1..552ab0db 100644
--- a/src/mp3.c
+++ b/src/mp3.c
@@ -680,26 +680,17 @@ static int startread(sox_format_t * ft)
 
 static void errorf(const char* fmt, va_list va)
 {
-  sox_globals.subsystem=__FILE__;
-  if (sox_globals.output_message_handler)
-    (*sox_globals.output_message_handler)(1,sox_globals.subsystem,fmt,va);
-  return;
+  lsx_fail(fmt, va);
 }
 
 static void debugf(const char* fmt, va_list va)
 {
-  sox_globals.subsystem=__FILE__;
-  if (sox_globals.output_message_handler)
-    (*sox_globals.output_message_handler)(4,sox_globals.subsystem,fmt,va);
-  return;
+  lsx_debug(fmt, va);
 }
 
 static void msgf(const char* fmt, va_list va)
 {
-  sox_globals.subsystem=__FILE__;
-  if (sox_globals.output_message_handler)
-    (*sox_globals.output_message_handler)(3,sox_globals.subsystem,fmt,va);
-  return;
+  lsx_report(fmt, va);
 }
 
 /* These functions are considered optional. If they aren't present in the
diff --git a/src/sox.h b/src/sox.h
index bac50354..b9cd9754 100644
--- a/src/sox.h
+++ b/src/sox.h
@@ -79,14 +79,14 @@ the variable being unused (especially in macro-generated code).
 
 /**
 Plugins API:
-LSX_PRINTF12: Attribute applied to a function to indicate that it requires
-a printf-style format string for arg1 and that printf parameters start at
-arg2.
+LSX_PRINTF23: Attribute applied to a function to indicate that it requires
+a printf-style format string for arg2 and that printf parameters start at
+arg3.
 */
 #ifdef __GNUC__
-#define LSX_PRINTF12  __attribute__ ((format (printf, 1, 2))) /* Function has printf-style arguments. */
+#define LSX_PRINTF23  __attribute__ ((format (printf, 2, 3))) /* Function has printf-style arguments. */
 #else
-#define LSX_PRINTF12 /* Function has printf-style arguments. */
+#define LSX_PRINTF23 /* Function has printf-style arguments. */
 #endif
 
 /**
@@ -1330,7 +1330,7 @@ typedef struct sox_globals_t {
 
   char const * stdin_in_use_by;  /**< Private: tracks the name of the handler currently using stdin */
   char const * stdout_in_use_by; /**< Private: tracks the name of the handler currently using stdout */
-  char const * subsystem;        /**< Private: tracks the name of the handler currently writing an output message */
+  char const * subsystem;        /**< Deprecated, Private: tracks the name of the handler currently writing an output message */
   char       * tmp_path;         /**< Private: client-configured path to use for temporary files */
   sox_bool     use_magic;        /**< Private: true if client has requested use of 'magic' file-type detection */
   sox_bool     use_threads;      /**< Private: true if client has requested parallel effects processing */
@@ -2273,9 +2273,10 @@ Print a fatal error in libSoX.
 void
 LSX_API
 lsx_fail_impl(
+    LSX_PARAM_IN_Z char const * filename, /**< Source code __FILE__ from which message originates. */
     LSX_PARAM_IN_PRINTF char const * fmt, /**< printf-style format string. */
     ...)
-    LSX_PRINTF12;
+    LSX_PRINTF23;
 
 /**
 Plugins API:
@@ -2284,9 +2285,10 @@ Print a warning in libSoX.
 void
 LSX_API
 lsx_warn_impl(
+    LSX_PARAM_IN_Z char const * filename, /**< Source code __FILE__ from which message originates. */
     LSX_PARAM_IN_PRINTF char const * fmt, /**< printf-style format string. */
     ...)
-    LSX_PRINTF12;
+    LSX_PRINTF23;
 
 /**
 Plugins API:
@@ -2295,9 +2297,10 @@ Print an informational message in libSoX.
 void
 LSX_API
 lsx_report_impl(
+    LSX_PARAM_IN_Z char const * filename, /**< Source code __FILE__ from which message originates. */
     LSX_PARAM_IN_PRINTF char const * fmt, /**< printf-style format string. */
     ...)
-    LSX_PRINTF12;
+    LSX_PRINTF23;
 
 /**
 Plugins API:
@@ -2306,33 +2309,34 @@ Print a debug message in libSoX.
 void
 LSX_API
 lsx_debug_impl(
+    LSX_PARAM_IN_Z char const * filename, /**< Source code __FILE__ from which message originates. */
     LSX_PARAM_IN_PRINTF char const * fmt, /**< printf-style format string. */
     ...)
-    LSX_PRINTF12;
+    LSX_PRINTF23;
 
 /**
 Plugins API:
 Report a fatal error in libSoX; printf-style arguments must follow.
 */
-#define lsx_fail       sox_get_globals()->subsystem=__FILE__,lsx_fail_impl
+#define lsx_fail(...)       lsx_fail_impl(__FILE__, __VA_ARGS__)
 
 /**
 Plugins API:
 Report a warning in libSoX; printf-style arguments must follow.
 */
-#define lsx_warn       sox_get_globals()->subsystem=__FILE__,lsx_warn_impl
+#define lsx_warn(...)       lsx_warn_impl(__FILE__, __VA_ARGS__)
 
 /**
 Plugins API:
 Report an informational message in libSoX; printf-style arguments must follow.
 */
-#define lsx_report     sox_get_globals()->subsystem=__FILE__,lsx_report_impl
+#define lsx_report(...)     lsx_report_impl(__FILE__, __VA_ARGS__)
 
 /**
 Plugins API:
 Report a debug message in libSoX; printf-style arguments must follow.
 */
-#define lsx_debug      sox_get_globals()->subsystem=__FILE__,lsx_debug_impl
+#define lsx_debug(...)      lsx_debug_impl(__FILE__, __VA_ARGS__)
 
 /**
 Plugins API:
diff --git a/src/sox_i.h b/src/sox_i.h
index c8552f97..c843556c 100644
--- a/src/sox_i.h
+++ b/src/sox_i.h
@@ -31,10 +31,10 @@
 #undef lsx_fail
 #undef lsx_report
 #undef lsx_warn
-#define lsx_debug sox_globals.subsystem=effp->handler.name,lsx_debug_impl
-#define lsx_fail sox_globals.subsystem=effp->handler.name,lsx_fail_impl
-#define lsx_report sox_globals.subsystem=effp->handler.name,lsx_report_impl
-#define lsx_warn sox_globals.subsystem=effp->handler.name,lsx_warn_impl
+#define lsx_debug(...) lsx_debug_impl(effp->handler.name, __VA_ARGS__)
+#define lsx_fail(...) lsx_fail_impl(effp->handler.name, __VA_ARGS__)
+#define lsx_report(...) lsx_report_impl(effp->handler.name, __VA_ARGS__)
+#define lsx_warn(...) lsx_warn_impl(effp->handler.name, __VA_ARGS__)
 #endif
 
 #define RANQD1 ranqd1(sox_globals.ranqd1)
@@ -56,11 +56,11 @@ assert_static(sizeof(off_t) == _FILE_OFFSET_BITS >> 3, OFF_T_BUILD_PROBLEM);
 
 FILE * lsx_tmpfile(void);
 
-void lsx_debug_more_impl(char const * fmt, ...) LSX_PRINTF12;
-void lsx_debug_most_impl(char const * fmt, ...) LSX_PRINTF12;
+void lsx_debug_more_impl(LSX_PARAM_IN_Z char const * filename, LSX_PARAM_IN_PRINTF char const * fmt, ...) LSX_PRINTF23;
+void lsx_debug_most_impl(LSX_PARAM_IN_Z char const * filename, LSX_PARAM_IN_PRINTF char const * fmt, ...) LSX_PRINTF23;
 
-#define lsx_debug_more sox_get_globals()->subsystem=__FILE__,lsx_debug_more_impl
-#define lsx_debug_most sox_get_globals()->subsystem=__FILE__,lsx_debug_most_impl
+#define lsx_debug_more(...) lsx_debug_more_impl(__FILE__, __VA_ARGS__)
+#define lsx_debug_most(...) lsx_debug_most_impl(__FILE__, __VA_ARGS__)
 
 /* Digitise one cycle of a wave and store it as
  * a table of samples of a specified data-type.
-- 
2.33.0.153.gba50c8fa24-goog


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]



[-- Attachment #4: Type: text/plain, Size: 158 bytes --]

_______________________________________________
SoX-devel mailing list
SoX-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sox-devel

                 reply	other threads:[~2021-09-08 21:23 UTC|newest]

Thread overview: [no followups] expand[flat|nested]  mbox.gz  Atom feed

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://lists.sourceforge.net/lists/listinfo/sox-devel

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAO9x7dKWc=_12-2mykAqMV8xFd_EMSVi6S6G3Pfu3GyAM1cdkQ@mail.gmail.com' \
    --to=sox-devel@lists.sourceforge.net \
    --cc=enzam@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/sox.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).