On Wed, 2018-05-16 at 11:08 +0200, Christophe de Dinechin wrote:
> > On 16 May 2018, at 10:59, Lukáš Hrázký <lhrazky@xxxxxxxxxx> 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@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 :)
>
> In my original proposal, I used “save_errno” to avoid the ugly trailing underscore ;-)
Yeah, the "saved_errno" just didn't seem great to me either, because
it's not really "saved" at the time you pass it as an argument to the
constructor... (might depend how you look at it though)
And I've seen trailing underscores used to differentiate variable names
before.
> sys_errno would be OK too.
Yeah, can be, if you guys can't bear to look at the underscore there :)
>
> >
> > > > + 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).
>
> Agree with both. Also, in theory you don’t need <errno.h> in the header.
Noted, probably a left-over.
> >
> > > > +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
>
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel