On Wed, Nov 01, 2017 at 01:54:17PM +0100, Berthold Stoeger wrote: > 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.
I don't /think/ that it's possible (because of the way the download UI prevents you from doing other things while the download thread is running), but I agree that we should just get this right 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. Linus will love that :-) > 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. I believe that's correct > 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. In general, our header files at some point need a patient person to go through and add some sense to them. Right now it's a hot mess. > 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 > 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 Well, for one thing the two consecutive ifdef seem odd, but as I said before, I'd rather have this in a different header file that is already only used by the two UIs (and therefore C++) > 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); > } This looks simple :-) > +extern "C" > int report_error(const char *fmt, ...) > { > - 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(); Other than these two requests, I think this looks fine. Any other comments? /D _______________________________________________ subsurface mailing list [email protected] http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
