[Spice-devel] [PATCH spice-streaming-agent 3/9] Implement an exception hierarchy for ReadError
Lukáš Hrázký
lhrazky at redhat.com
Wed May 16 09:55:05 UTC 2018
On Wed, 2018-05-16 at 05:25 -0400, Frediano Ziglio wrote:
> >
> > 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.
I prefer c3d's "sys_errno" to those.
> > > > + 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?
Right, good catch. IIRC c3d didn't want to inherit from runtime_error,
again IIRC because it is meant for different kind of errors or
something like this. I'm fine with actually using runtime_error, the
C++ exception hierarchy is convoluted and I don't think a clean
solution is easily achievable (Enough to look at logic_error, it's
derived classes and when those are thrown).
> > > > +};
> > > > +
> > > > +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).
Cool, for some weird reason I thought inheriting constructors are not
in C++11. I'll fix it, thanks.
> > > > +
> > > > +}} // 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
More information about the Spice-devel
mailing list