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

[Lukáš Hrázký]

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 :)

> > +    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;
> > +};
> > +
> > +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_);
> > +};
> > +
> > +}} // 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