[Spice-devel] [PATCH spice-streaming-agent 3/9] Implement an exception hierarchy for ReadError

[Frediano Ziglio]

> 
> On Tue, 2018-05-15 at 16:37 -0400, Frediano Ziglio wrote:
> > > 
> > > Introduces an exception hierarchy up to a ReadError class, which is
> > > thrown from read_all().
> > > 
> > > Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> > > ---
> > >  src/Makefile.am               |  2 ++
> > >  src/error.cpp                 | 29 ++++++++++++++++++++++++++++
> > >  src/error.hpp                 | 44
> > >  +++++++++++++++++++++++++++++++++++++++++++
> > >  src/spice-streaming-agent.cpp |  2 +-
> > >  src/stream-port.cpp           |  4 ++--
> > >  5 files changed, 78 insertions(+), 3 deletions(-)
> > >  create mode 100644 src/error.cpp
> > >  create mode 100644 src/error.hpp
> > > 
> > > diff --git a/src/Makefile.am b/src/Makefile.am
> > > index 604c1e5..18ed22c 100644
> > > --- a/src/Makefile.am
> > > +++ b/src/Makefile.am
> > > @@ -52,6 +52,8 @@ spice_streaming_agent_SOURCES = \
> > >  	spice-streaming-agent.cpp \
> > >  	concrete-agent.cpp \
> > >  	concrete-agent.hpp \
> > > +	error.cpp \
> > > +	error.hpp \
> > >  	mjpeg-fallback.cpp \
> > >  	mjpeg-fallback.hpp \
> > >  	jpeg.cpp \
> > > diff --git a/src/error.cpp b/src/error.cpp
> > > new file mode 100644
> > > index 0000000..1b76ea4
> > > --- /dev/null
> > > +++ b/src/error.cpp
> > > @@ -0,0 +1,29 @@
> > > +/* The errors module.
> > > + *
> > > + * \copyright
> > > + * Copyright 2018 Red Hat Inc. All rights reserved.
> > > + */
> > > +
> > > +#include "error.hpp"
> > > +
> > > +#include <string.h>
> > > +#include <syslog.h>
> > > +
> > > +
> > > +namespace spice {
> > > +namespace streaming_agent {
> > > +
> > > +Error::Error(const std::string &message) : message(message) {}
> > > +
> > > +const char* Error::what() const noexcept
> > > +{
> > > +    return message.c_str();
> > > +}
> > > +
> > > +IOError::IOError(const std::string &msg, int errno_) :
> > 
> > I personally don't like much the errno_, maybe a sys_error?
> 
> Well, with "errno_", you know it's supposed to be a errno value, with
> "sys_error", you are left guessing at what number it should be and
> would need additional documentation (well, I suppose some documentation
> wouldn't hurt anyway :) but these are trivial interfaces). So I prefer
> the self-documenting name with and ugly trailing underscore :)
> 

save_errno as proposed or err_no ?
Not strong on it.

> > > +    Error(msg + ": " + std::to_string(errno_) + " - " +
> > > strerror(errno_))
> > > +{}
> > > +
> > > +ReadError::ReadError(const std::string &msg, int errno_) : IOError(msg,
> > > errno_) {}
> > > +
> > 
> > Not strong opinion, maybe format the same way the other constructor?
> 
> You mean a line-break after the colon? I like to keep things on one
> line (and save on vertical space) if possible, but our coding style
> likes to spread the code vertically :) If we agree on a line-break
> after the colon, fine by me.
> 
> > > +}} // namespace spice::streaming_agent
> > > diff --git a/src/error.hpp b/src/error.hpp
> > > new file mode 100644
> > > index 0000000..de1cb83
> > > --- /dev/null
> > > +++ b/src/error.hpp
> > > @@ -0,0 +1,44 @@
> > > +/* The errors module.
> > > + *
> > > + * \copyright
> > > + * Copyright 2018 Red Hat Inc. All rights reserved.
> > > + */
> > > +
> > > +#ifndef SPICE_STREAMING_AGENT_ERROR_HPP
> > > +#define SPICE_STREAMING_AGENT_ERROR_HPP
> > > +
> > > +#include <exception>
> > > +#include <string>
> > > +
> > > +
> > > +namespace spice {
> > > +namespace streaming_agent {
> > > +
> > > +class Error : public std::exception
> > > +{
> > > +public:
> > > +    Error(const std::string &message);
> > > +    const char *what() const noexcept override final;
> > > +
> > > +protected:
> > > +    const std::string message;

Note that the copy constructor should not throw exceptions but std::string
copy constructor potentially does.
Would not be easier to inherit from std::runtime_error which already
handle the string and copy construction?

> > > +};
> > > +
> > > +class IOError : public Error
> > > +{
> > > +public:
> > > +    IOError(const std::string &msg, int errno_);
> > > +
> > 
> > Maybe the error count have a default value, something like:
> > 
> >     IOError(const std::string &msg, int sys_error = errno);
> 
> Two reasons why not:
> 1. The exception doesn't necessarily need to be constructed right after
> the call that set the errno to the error we want to throw, I prefer to
> make it explicit so that someone doesn't accidentally use the exception
> with a wrong errno.
> 
> 2. I introduce a IOError(const std::string &msg) constructor in patch
> 7/9, because not all errors have an errno (the poll() ones don't).
> 
> > > +protected:
> > > +    int errno_;
> > > +};
> > > +
> > > +class ReadError : public IOError
> > > +{
> > > +public:
> > > +    ReadError(const std::string &msg, int errno_);
> > > +};

Why not using constructor inheritance?

class ReadError : public IOError
{
public:
    using IOError::IOError;
};

this avoid having to define the constructor again and adding
a default constructor (patch 7/9 will require less changes).

> > > +
> > > +}} // namespace spice::streaming_agent
> > > +
> > > +#endif // SPICE_STREAMING_AGENT_ERROR_HPP
> > > diff --git a/src/spice-streaming-agent.cpp
> > > b/src/spice-streaming-agent.cpp
> > > index 7b166d3..2c0340d 100644
> > > --- a/src/spice-streaming-agent.cpp
> > > +++ b/src/spice-streaming-agent.cpp
> > > @@ -535,7 +535,7 @@ int main(int argc, char* argv[])
> > >      try {
> > >          do_capture(streamport, f_log);
> > >      }
> > > -    catch (std::runtime_error &err) {
> > > +    catch (std::exception &err) {
> > >          syslog(LOG_ERR, "%s\n", err.what());
> > >          ret = EXIT_FAILURE;
> > >      }
> > > diff --git a/src/stream-port.cpp b/src/stream-port.cpp
> > > index 3699d92..f256698 100644
> > > --- a/src/stream-port.cpp
> > > +++ b/src/stream-port.cpp
> > > @@ -5,6 +5,7 @@
> > >   */
> > >  
> > >  #include "stream-port.hpp"
> > > +#include "error.hpp"
> > >  
> > >  #include <errno.h>
> > >  #include <string.h>
> > > @@ -25,8 +26,7 @@ void read_all(int fd, void *msg, size_t len)
> > >              if (errno == EINTR) {
> > >                  continue;
> > >              }
> > > -            throw std::runtime_error("Reading message from device
> > > failed: "
> > > +
> > > -                                     std::string(strerror(errno)));
> > > +            throw ReadError("Reading message from device failed",
> > > errno);
> > >          }
> > >  
> > >          len -= n;
> > 
> > Beside that,
> > 
> > Acked-by: Frediano Ziglio <fziglio at redhat.com>
> > 

Frediano