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

[Lukáš Hrázký]

On Thu, 2018-02-22 at 10:35 +0100, Christophe de Dinechin wrote:
> > 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.

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?

> 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;
> 
>