[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