[Spice-devel] [PATCH spice-streaming-agent 3/9] Implement an exception hierarchy for ReadError
Christophe de Dinechin
cdupontd at redhat.com
Wed May 16 09:52:39 UTC 2018
> On 16 May 2018, at 11:25, Frediano Ziglio <fziglio 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 :)
>>
>
> save_errno as proposed or err_no ?
:-) [Note to Jonathon: wouldn't this be read as "err… no!" ???]
> Not strong on it.
Neither am I, errno_ would be OK with me if it wasn’t a “first” in the SPICe code.
>
>>>> + 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 at redhat.com>
>>>
>
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list