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