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

[Christophe de Dinechin]

> On 16 May 2018, at 10:59, Lukáš Hrázký <lhrazky at redhat.com> 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 :)

In my original proposal, I used “save_errno” to avoid the ugly trailing underscore ;-)

sys_errno would be OK too.



> 
>>> +    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.

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