On Mittwoch, 1. November 2017 15:28:28 CET Dirk Hohndel wrote:
> On Wed, Nov 01, 2017 at 01:54:17PM +0100, Berthold Stoeger wrote:
> > 1) errorhelper.c is turned into C++ code.
>
> Linus will love that :-)
I sense some sarcasm. ;)
I can relate. When using a full blown cross-platform toolkit like Qt you can
expect some contagiousness. I have a feeling that Qt is particularly
contagious, for better or worse.
> > This necessitates a kludge of declaring get_error_string() only for C++
> > code and makes dive.h inconsistent.
>
> Well, if it's only used in C++ code then it maybe shouldn't be declared in
> dive.h in the first place.
"core/helpers.h" seems like the place for C++ helpers.
> > - struct membuffer *buf = &error_string_buffer;
> > + {
>
> Why is there another indentation / opening brace?
>
> > + 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);
> > + }
>
> My guess is you are doing this to destroy the QMutexLocker. I think the
> code would be much easier to read if you simply called lock->unlock();
Indeed... AFAIK this is idiomatic (or idiosyncratic?) C++. The idea is that
any function call can throw an exception. The brace-close makes the lock-guard
go out of scope and thus releases the lock.
Since this is no-exception code, I replaced it by a .lock()/.unlock() pair,
which is hopefully easier on the eyes.
Updated patch attached.
Berthold>From 41fd34eb88359a617a4c34821f63271096c8fc7d 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 | 1 -
core/{errorhelper.c => errorhelper.cpp} | 19 ++++++++++++++++---
core/helpers.h | 1 +
desktop-widgets/mainwindow.cpp | 5 +++--
mobile-widgets/qmlmanager.cpp | 1 +
smtk-import/smrtk2ssrfc_window.cpp | 3 ++-
7 files changed, 24 insertions(+), 8 deletions(-)
rename core/{errorhelper.c => errorhelper.cpp} (77%)
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..04ce4cc0 100644
--- a/core/dive.h
+++ b/core/dive.h
@@ -712,7 +712,6 @@ extern "C" {
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 77%
rename from core/errorhelper.c
rename to core/errorhelper.cpp
index 66c01fd2..78ca7e36 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 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,30 @@ 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;
/* Previous unprinted errors? Add a newline in between */
+ error_string_buffer_mutex.lock();
if (buf->len)
put_bytes(buf, "\n", 1);
VA_BUF(buf, fmt);
mb_cstring(buf);
+ error_string_buffer_mutex.unlock();
/* if an error callback is registered, call it */
if (error_cb)
@@ -45,11 +56,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/core/helpers.h b/core/helpers.h
index 06c1c852..22865a04 100644
--- a/core/helpers.h
+++ b/core/helpers.h
@@ -46,6 +46,7 @@ QString uiLanguage(QLocale *callerLoc);
QLocale getLocale();
void selectedDivesGasUsed(QVector<QPair<QString, int> > &gasUsed);
QString getUserAgent();
+QString get_error_string();
#if defined __APPLE__
#define TITLE_OR_TEXT(_t, _m) "", _t + "\n" + _m
diff --git a/desktop-widgets/mainwindow.cpp b/desktop-widgets/mainwindow.cpp
index bcd8f8e6..033e9dff 100644
--- a/desktop-widgets/mainwindow.cpp
+++ b/desktop-widgets/mainwindow.cpp
@@ -41,6 +41,7 @@
#include "desktop-widgets/usersurvey.h"
#include "core/divesitehelpers.h"
#include "core/windowtitleupdate.h"
+#include "core/helpers.h"
#include "desktop-widgets/locationinformation.h"
#include "preferences/preferencesdialog.h"
@@ -100,8 +101,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/mobile-widgets/qmlmanager.cpp b/mobile-widgets/qmlmanager.cpp
index b0e597f6..41011f03 100644
--- a/mobile-widgets/qmlmanager.cpp
+++ b/mobile-widgets/qmlmanager.cpp
@@ -22,6 +22,7 @@
#include "core/divelist.h"
#include "core/device.h"
#include "core/pref.h"
+#include "core/helpers.h"
#include "core/qthelper.h"
#include "core/qt-gui.h"
#include "core/git-access.h"
diff --git a/smtk-import/smrtk2ssrfc_window.cpp b/smtk-import/smrtk2ssrfc_window.cpp
index 98734cf4..1bf0b41d 100644
--- a/smtk-import/smrtk2ssrfc_window.cpp
+++ b/smtk-import/smrtk2ssrfc_window.cpp
@@ -3,6 +3,7 @@
#include "ui_smrtk2ssrfc_window.h"
#include "qt-models/filtermodels.h"
#include "core/dive.h"
+#include "core/helpers.h"
#include "core/divelist.h"
#include <QFileDialog>
#include <QFileInfo>
@@ -82,7 +83,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
