[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 11:01 +0100, Christophe de Dinechin wrote:
> > On Feb 22, 2018, at 10:51 AM, Lukáš Hrázký <lhrazky at 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ý <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?
> 
> 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.

Ok, I mostly agree, possibly with the exception of plugin init. I think
skipping the plugin and keep going might be more robust. Sure, the
plugin will not be unloaded properly, which may cause subsequent errors
elsewhere, but there should be enough information to debug the issue
and the agent is likely to keep working.

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

I'm inclined to agree too, core dump would certainly be handy for us.

Lukas

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