Dear all,
At the moment errors can be registered with the report_error() function.
Multiple errors are concatenated with a '\n' separator. The errors are read/
reset with get_error_string(). This is potentially racy, as these functions
may be called from different threads. It is unclear whether such a situation
can actually arise, but nevertheless the problem should be fixed as a matter of
principle.
Here is my attempt at protecting the underlying membuffer with a mutex. This
code introduces two notable changes:
1) errorhelper.c is turned into C++ code.
The rationale here is that the platform-independent QMutex class is used. I'm
not aware of a portable mutex in the C standard?
2) The return type of get_error_string() is changed from const char * to
QString.
The problem with returning a const char * is that the buffer might be
overwritten by a different thread. So one would have to strdup() the buffer.
But this in return would mean that the caller has to free() the buffer.
Changing this to QString makes for a drop-in replacement that needs no further
changes. Indeed, the way I read it, all callers converted the output of
get_error_string() to a QString anyway.
This necessitates a kludge of declaring get_error_string() only for C++ code
and makes dive.h inconsistent.
So what do you think?
Berthold
>From 9e7f35b0fa3ee5865700e4fa17204f734f6b1b1b Mon Sep 17 00:00:00 2001
From: Berthold Stoeger <[email protected]>
Date: Wed, 1 Nov 2017 09:30:56 +0100
Subject: [PATCH] Make the report_error(), get_error_string() code thread safe
report_error() collects error messages which can be read by
get_error_string(). This code is called from different threads.
Although unlikely, in principle these accesses could happen at
the same time. Therefore, it should be made thread safe.
1) The errorhelper.c code was transformed into C++.
Rationale: for cross-platform mutexes the Qt Mutex and MutexLocker
classes are used.
2) A static QMutex was added to protect the membuffer.
3) The get_error_string() function returns a QString.
For this to work, the get_error_string() function must only be
declared for C++ code.
Rationale: A copy of the string must be returned, because the
membuffer might be changed by a different thread. Returning a
strdup()ed C string would mean that all call sites delete this string.
Moreover, currently all callers convert the C-string directly into
a QString anyway.
Signed-off-by: Berthold Stoeger <[email protected]>
---
core/CMakeLists.txt | 2 +-
core/dive.h | 6 +++++-
core/{errorhelper.c => errorhelper.cpp} | 32 +++++++++++++++++++++++---------
desktop-widgets/mainwindow.cpp | 4 ++--
smtk-import/smrtk2ssrfc_window.cpp | 2 +-
5 files changed, 32 insertions(+), 14 deletions(-)
rename core/{errorhelper.c => errorhelper.cpp} (66%)
diff --git a/core/CMakeLists.txt b/core/CMakeLists.txt
index f063f3aa..e47b15aa 100644
--- a/core/CMakeLists.txt
+++ b/core/CMakeLists.txt
@@ -40,7 +40,7 @@ set(SUBSURFACE_CORE_LIB_SRCS
divesite.cpp
divelist.c
equipment.c
- errorhelper.c
+ errorhelper.cpp
file.c
gas-model.c
git-access.c
diff --git a/core/dive.h b/core/dive.h
index d3b75549..b18ecf62 100644
--- a/core/dive.h
+++ b/core/dive.h
@@ -706,13 +706,17 @@ static inline int dive_has_gps_location(struct dive *dive)
return dive_site_has_gps_location(get_dive_site_by_uuid(dive->dive_site_uuid));
}
+#ifdef __cplusplus
+#include <QString>
+extern QString get_error_string(void);
+#endif
+
#ifdef __cplusplus
extern "C" {
#endif
extern int report_error(const char *fmt, ...);
extern void report_message(const char *msg);
-extern const char *get_error_string(void);
extern void set_error_cb(void(*cb)(void));
extern struct dive *find_dive_including(timestamp_t when);
diff --git a/core/errorhelper.c b/core/errorhelper.cpp
similarity index 66%
rename from core/errorhelper.c
rename to core/errorhelper.cpp
index 66c01fd2..0834596a 100644
--- a/core/errorhelper.c
+++ b/core/errorhelper.cpp
@@ -3,11 +3,18 @@
// Clang has a bug on zero-initialization of C structs.
#pragma clang diagnostic ignored "-Wmissing-field-initializers"
#endif
+
+#include <QMutex>
+#include <QMutexLocker>
+
#include "dive.h"
#include "membuffer.h"
#define VA_BUF(b, fmt) do { va_list args; va_start(args, fmt); put_vformat(b, fmt, args); va_end(args); } while (0)
+// Mutex to protect access error_string_buffer
+static QMutex error_string_buffer_mutex;
+
static struct membuffer error_string_buffer = { 0 };
static void (*error_cb)(void) = NULL;
/*
@@ -17,26 +24,31 @@ static void (*error_cb)(void) = NULL;
* error reports will overwrite the string rather than
* append to it.
*/
-const char *get_error_string(void)
+QString get_error_string(void)
{
const char *str;
+ QMutexLocker lock(&error_string_buffer_mutex);
if (!error_string_buffer.len)
- return "";
+ return QString();
str = mb_cstring(&error_string_buffer);
error_string_buffer.len = 0;
- return str;
+ return QString(str);
}
+extern "C"
int report_error(const char *fmt, ...)
{
- struct membuffer *buf = &error_string_buffer;
+ {
+ QMutexLocker lock(&error_string_buffer_mutex);
+ struct membuffer *buf = &error_string_buffer;
- /* Previous unprinted errors? Add a newline in between */
- if (buf->len)
- put_bytes(buf, "\n", 1);
- VA_BUF(buf, fmt);
- mb_cstring(buf);
+ /* Previous unprinted errors? Add a newline in between */
+ if (buf->len)
+ put_bytes(buf, "\n", 1);
+ VA_BUF(buf, fmt);
+ mb_cstring(buf);
+ }
/* if an error callback is registered, call it */
if (error_cb)
@@ -45,11 +57,13 @@ int report_error(const char *fmt, ...)
return -1;
}
+extern "C"
void report_message(const char *msg)
{
(void)report_error("%s", msg);
}
+extern "C"
void set_error_cb(void(*cb)(void)) {
error_cb = cb;
}
diff --git a/desktop-widgets/mainwindow.cpp b/desktop-widgets/mainwindow.cpp
index bcd8f8e6..1adf8235 100644
--- a/desktop-widgets/mainwindow.cpp
+++ b/desktop-widgets/mainwindow.cpp
@@ -100,8 +100,8 @@ extern "C" void showErrorFromC()
void MainWindow::showErrors()
{
- const char *error = get_error_string();
- if (error && error[0])
+ QString error = get_error_string();
+ if (!error.isEmpty())
getNotificationWidget()->showNotification(error, KMessageWidget::Error);
}
diff --git a/smtk-import/smrtk2ssrfc_window.cpp b/smtk-import/smrtk2ssrfc_window.cpp
index 98734cf4..a1b4b7c3 100644
--- a/smtk-import/smrtk2ssrfc_window.cpp
+++ b/smtk-import/smrtk2ssrfc_window.cpp
@@ -82,7 +82,7 @@ void Smrtk2ssrfcWindow::on_importButton_clicked()
ui->progressBar->setValue(i);
fileNamePtr = QFile::encodeName(inputFiles.at(i));
smartrak_import(fileNamePtr.data(), &dive_table);
- ui->plainTextEdit->appendPlainText(QString(get_error_string()));
+ ui->plainTextEdit->appendPlainText(get_error_string());
}
ui->progressBar->setValue(inputFiles.size());
save_dives_logic(outputFile.toUtf8().data(), false);
--
2.14.1
_______________________________________________
subsurface mailing list
[email protected]
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
