[RFC] Make the report_error(), get_error_string() code thread safe
Dirk Hohndel
dirk at hohndel.org
Wed Nov 1 07:28:28 PDT 2017
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 <bstoeger at mail.tuwien.ac.at>
> 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
More information about the subsurface
mailing list