[Spice-devel] [PATCH spice-streaming-agent 4/9] Introduce a WriteError exception for write_all()

[Lukáš Hrázký]

On Wed, 2018-05-16 at 05:46 -0400, Frediano Ziglio wrote:
> > 
> > On Tue, 2018-05-15 at 16:42 -0400, Frediano Ziglio wrote:
> > > > 
> > > > Update the interface to not return the size written, as it is not needed
> > > > anymore.
> > > > 
> > > > Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> > > > ---
> > > >  src/error.cpp                 |  3 ++-
> > > >  src/error.hpp                 | 14 +++++++++++++
> > > >  src/spice-streaming-agent.cpp | 48
> > > >  +++++++++++++++----------------------------
> > > >  src/stream-port.cpp           |  7 ++-----
> > > >  src/stream-port.hpp           |  2 +-
> > > >  5 files changed, 35 insertions(+), 39 deletions(-)
> > > > 
> > > > diff --git a/src/error.cpp b/src/error.cpp
> > > > index 1b76ea4..4ef275c 100644
> > > > --- a/src/error.cpp
> > > > +++ b/src/error.cpp
> > > > @@ -7,7 +7,6 @@
> > > >  #include "error.hpp"
> > > >  
> > > >  #include <string.h>
> > > > -#include <syslog.h>
> > > >  
> > > >  
> > > >  namespace spice {
> > > > @@ -26,4 +25,6 @@ IOError::IOError(const std::string &msg, int errno_) :
> > > >  
> > > >  ReadError::ReadError(const std::string &msg, int errno_) : IOError(msg,
> > > >  errno_) {}
> > > >  
> > > > +WriteError::WriteError(const std::string &msg, int errno_) :
> > > > IOError(msg,
> > > > errno_) {}
> > > > +
> > > 
> > > Some comment on errno_ of previous patch.
> > > 
> > > >  }} // namespace spice::streaming_agent
> > > > diff --git a/src/error.hpp b/src/error.hpp
> > > > index de1cb83..d69942c 100644
> > > > --- a/src/error.hpp
> > > > +++ b/src/error.hpp
> > > > @@ -9,6 +9,7 @@
> > > >  
> > > >  #include <exception>
> > > >  #include <string>
> > > > +#include <syslog.h>
> > > >  
> > > >  
> > > >  namespace spice {
> > > > @@ -39,6 +40,19 @@ public:
> > > >      ReadError(const std::string &msg, int errno_);
> > > >  };
> > > >  
> > > > +class WriteError : public IOError
> > > > +{
> > > > +public:
> > > > +    WriteError(const std::string &msg, int errno_);
> > > > +};
> > > > +
> > > > +template<class T>
> > > > +const T &syslog(const T &e) noexcept
> > > 
> > > Why not accepting a const std::exception &e instead of the template?
> > 
> > Because then it wouldn't automatically deduce the T type and you'd have
> > to write it like:
> > 
> > throw syslog<MyException>(MyException());
> > 
> > Note that the T is there because you need to return the actual type of
> > the exception, not a predecessor (which was the version in c3d's patch,
> > where syslog() was a method of the Error class), because if you don't
> > throw it as the actual type, you then can't catch it as the actual
> > type.
> > 
> 
> Didn't though at that usage of the function.
> Usually you throw the exception and the handler is responsible to
> use the message.

Usually yeah, but sometimes you want to log the error at the place it
was created, because if you catch exceptions, it's impossible (AFAIK)
to get the traceback (unless you use a special lib to do that, a whole
another topic). So I find this actually a pretty elegant way to allow
you the flexibility to both throw and log+throw on one line. You could
even log only with just:

syslog(MyException());

:) :)

> In this way is not possible to ignore the error entirely.

Not following here.

> Not sure I like much this usage, surely need some more comment on the code.

I think it's quite regular C++ way of doing things. What exactly should
be commented?

> [snip]
> 
> Frediano