> 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 ;-)
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@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