Thread related crash on failed dive computer download

[Berthold Stoeger]

On Dienstag, 31. Oktober 2017 19:25:03 CET Dirk Hohndel wrote:

> On Tue, Oct 31, 2017 at 03:52:28PM +0100, Berthold Stoeger wrote:
> > As a FYI, the patch attached, though without commit-message because it is
> > simply too ugly. It works for my particular test case, but...
> 
> Why is it too ugly?

Duplication of the va_args thing. At a place where it doesn't belong.
Movement of C-style strings to QString back to C-style strings back to 
QStrings. Oh my.
No encapsulation whatsoever. C function defined in C++ header and declared 
extern in C source.
Etc. :(

> > +	errorList.setLocalData(QStringList());
> 
> So we initialize to an empty list. I'm unclear if there could be messages
> still in that local storage that haven't been delivered, yet.

You're right, the local storage should be gone once the thread exits, even 
though the thread object is reused.

What should be cleared though is the "errors" member, because the object is 
reused.

BTW: Another bug: the code doesn't clear the membuffer.

> >  	if (errorText)
> >  	
> >  		error = str_error(errorText, internalData->devname,
> >  		internalData->vendor, internalData->product);> 
> > +	errors = std::move(errorList.localData());
> > +
>
> Because this seems like it would ('move'?) clear out the local storage,
> correct?

Forget the std::move - it should be removed. This would be used in standard C+
+ where list assignment makes a deep copy. A std::move on the other hand just 
replaces pointers. But as I just learned, Qt containers do copy on write, so 
this is not necessary. I have no idea how they would react to a std::move.

> > +
> > +// Dirty hack: report_error(...) crashed when not called from main
> > thread.
> > +// Thus, replace report_error(...) calls by report_error_thread(...)
> > +// calls, which collect error messages in thread local storage.
> > +extern "C" int report_error_thread(const char *fmt, ...);
> > +
> 
> Again, why is this a dirty hack?

See above.

> > +// Hack! for report_error_thread()
> > +#include "core/downloadfromdcthread.h"
> 
> Why is that a hack?

Because downloadfromdcthread.h should not be needed here?

> > @@ -220,6 +220,9 @@ void DownloadFromDCWidget::updateState(states state)
> > 
> >  	// got an error
> >  	else if (state == ERROR) {
> >  	
> >  		timer->stop();
> > 
> > +		foreach(const QString &s, thread.errors) {
> > +			report_message(s.toUtf8().data());
> > +		}
> 
> This one I don't understand... why aren't these reported when we do the
> std::move() above?

On completion, the thread moved the messages collected in the thread local 
area to the "errors" member of the thread object. And here the main thread 
displays the collected messages.

To be honest, apart from the errors mentioned above, I'm not even sure that 
the idea behind the code is sound. If you want, I can send an updated/cleaned 
up patch, but I have a bad feeling with this one. There probably should at 
least be a non-varargs version.

Berthold