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

2018-02-22 Thread 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ý  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ý  wrote:
> > > > 
> > > > On Wed, 2018-02-21 at 18:46 +0100, Christophe de Dinechin wrote:
> > > > > From: Christophe de Dinechin 
> > > > > 
> > > > > I also added a catch-all clause for double plus extra safety
> > > > > 
> > > > > Signed-off-by: Christophe de Dinechin 
> > > > > ---
> > > > > 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 ) {
> > > > > +catch (std::exception ) {
> > > > >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;
> 
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-22 Thread Christophe de Dinechin 

> On Feb 22, 2018, at 10:51 AM, Lukáš Hrázký  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ý  wrote:
>>> 
>>> On Wed, 2018-02-21 at 18:46 +0100, Christophe de Dinechin wrote:
 From: Christophe de Dinechin 
 
 I also added a catch-all clause for double plus extra safety
 
 Signed-off-by: Christophe de Dinechin 
 ---
 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 ) {
 +catch (std::exception ) {
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

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

2018-02-22 Thread 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ý  wrote:
> > 
> > On Wed, 2018-02-21 at 18:46 +0100, Christophe de Dinechin wrote:
> > > From: Christophe de Dinechin 
> > > 
> > > I also added a catch-all clause for double plus extra safety
> > > 
> > > Signed-off-by: Christophe de Dinechin 
> > > ---
> > > 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 ) {
> > > +catch (std::exception ) {
> > > 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;
> 
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-22 Thread Christophe de Dinechin 

> On Feb 22, 2018, at 10:23 AM, Lukáš Hrázký  wrote:
> 
> On Wed, 2018-02-21 at 18:46 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin 
>> 
>> I also added a catch-all clause for double plus extra safety
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> 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 ) {
>> +catch (std::exception ) {
>> 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;

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

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

2018-02-22 Thread Lukáš Hrázký 
On Wed, 2018-02-21 at 18:46 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> I also added a catch-all clause for double plus extra safety
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  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 ) {
> +catch (std::exception ) {
>  syslog(LOG_ERR, "%s\n", err.what());
>  ret = EXIT_FAILURE;
>  }
> +catch (...) {
> +syslog(LOG_ERR, "Unexpected exception caught (probably thronw by 
> plugin init");

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

I don't like the speculation in the error message. 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).

Lukas

> +ret = EXIT_FAILURE;
> +}
>  
>  closelog();
>  return ret;
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-21 Thread Christophe de Dinechin 
From: Christophe de Dinechin 

I also added a catch-all clause for double plus extra safety

Signed-off-by: Christophe de Dinechin 
---
 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 ) {
+catch (std::exception ) {
 syslog(LOG_ERR, "%s\n", err.what());
 ret = EXIT_FAILURE;
 }
+catch (...) {
+syslog(LOG_ERR, "Unexpected exception caught (probably thronw by 
plugin init");
+ret = EXIT_FAILURE;
+}
 
 closelog();
 return ret;
-- 
2.13.5 (Apple Git-94)

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