>
> 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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx>
> >
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel