> On Feb 22, 2018, at 10:51 AM, Lukáš Hrázký <lhra...@redhat.com> wrote:
> 
> On Thu, 2018-02-22 at 10:35 +0100, Christophe de Dinechin wrote:
>>> On Feb 22, 2018, at 10:23 AM, Lukáš Hrázký <lhra...@redhat.com> wrote:
>>> 
>>> On Wed, 2018-02-21 at 18:46 +0100, Christophe de Dinechin wrote:
>>>> From: Christophe de Dinechin <dinec...@redhat.com>
>>>> 
>>>> I also added a catch-all clause for double plus extra safety
>>>> 
>>>> Signed-off-by: Christophe de Dinechin <dinec...@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.
> 
> I would be interested in the reasoning. Are we still talking about
> plugin init? For example if it throws 'int'? I mean it could have done
> all sorts of ugly things and no cleanup before throwing the 'int', but
> it could have done all those things and _not_ throw even that 'int' too
> :) Or do you mean something else?

I’ll let Frediano share his point of view. For me, there are two cases:

1) Some std::exception such as bad_alloc. We should be able to deal with them 
in some reasonable way and recover, but we may not have a very good recovery 
strategy at the process level, e.g. the system may be low on memory or 
something like that. So it’s probably better dealt with the program exiting 
anyway. In any case, we need to catch exception to display the error message, 
no real point in aborting or core dumping if all that happened was some 
out-of-memory condition.

2) Some unknown exception, such as 42. More likely an vendor driver exception 
of some sort. I think it would be good to know what type was thrown and have a 
core dump. So maybe not catch(…) and let terminate() do its job.

> 
>> 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…
> 
> That sure is a dilemma I don't have a good answer to...
> 
>>> 
>>> Lukas
>>> 
>>>> +        ret = EXIT_FAILURE;
>>>> +    }
>>>> 
>>>>    closelog();
>>>>    return ret;

_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Reply via email to