[Spice-devel] [PATCH v2 18/24] Catch all std::exception derivatives (including std::bad_alloc, not a std::bad_alloc)

[Christophe de Dinechin]

> On Feb 22, 2018, at 10:23 AM, Lukáš Hrázký <lhrazky at redhat.com> wrote:
> 
> On Wed, 2018-02-21 at 18:46 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin at redhat.com>
>> 
>> I also added a catch-all clause for double plus extra safety
>> 
>> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
>> ---
>> src/spice-streaming-agent.cpp | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index f9d8855..26fa910 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -623,10 +623,14 @@ int main(int argc, char* argv[])
>>         FrameLog frame_log(log_filename, log_binary);
>>         agent.CaptureLoop(streamfd, frame_log);
>>     }
>> -    catch (std::runtime_error &err) {
>> +    catch (std::exception &err) {
>>         syslog(LOG_ERR, "%s\n", err.what());
>>         ret = EXIT_FAILURE;
>>     }
>> +    catch (...) {
>> +        syslog(LOG_ERR, "Unexpected exception caught (probably thronw by plugin init");
> 
> Typo.

Two, actually…

> Is it still technically an exception if it is not derived from
> std::exception? I wouldn't call it so, it can be std::string.

Well, I think it’s still called an exception even if it’s not std::exception, much like we talk about “functions” or “classes”. What matters is that it is thrown.

> 
> I don't like the speculation in the error message.

Hmmm, you are right. What was I thinking ;-) Clearly, given the two typos and the nonsensical message, I was rushing this a bit too much.


> What we should
> really do is catch exceptions from plugin init directly and log it
> accordingly (and also not necessarily abort the agent, let it run
> without the faulty plugin).

Well, I had a discussion with Frediano on this. He pointed out that things that do not derive from runtime_error are often hard to recover from. So unless we know how to recover, I think it’s a good thing to pop them back at the top and show a message.

We could debate whether a catch-all is a good things.

- Plus side: it lets us send a message to the syslog instead of stderr. Does that matter?
- Minus side: the message is not as clear as, say "terminate called after throwing an instance of ‘int’”, and no abort, so no core dump.

Maybe it’s not a good idea after all…



> 
> Lukas
> 
>> +        ret = EXIT_FAILURE;
>> +    }
>> 
>>     closelog();
>>     return ret;