> On 1 Mar 2018, at 15:46, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
>
> On Thu, 2018-03-01 at 12:40 +0100, Lukáš Hrázký wrote:
>> On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
>>> From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
>>>
>>> Throwing 'runtime_error' directly should be reserved for the support
>>> library. Add an 'Error' class as a base class for all errors thrown
>>> by the streaming agent, as well as subclasses used to discriminate
>>> between categories of error.
>>>
>>> The Error class offers:
>>> - a 'format_message' member function that formats a message without
>>> allocating memory, so that there is no risk of throwing another
>>> exception at throw time.
>>>
>>> - a 'syslog' member function that sends the formatted message to the syslog.
>>>
>>> The derived classes are:
>>>
>>> - IOError encapsulates typical I/O errors that report errors using
>>> errno. The format_message in that case prints the original message,
>>> along with the strerror message.
>>>
>>> - The ReadError and WriteError are two classes derived from IOError,
>>> with the sole purpose of making it possible to discriminate the
>>> error type at catch time, e.g. to retry writes.
>>>
>>> These classes are used in the subsequent patches
>>>
>>> Signed-off-by: Christophe de Dinechin <dinechin@xxxxxxxxxx>
>>> ---
>>> include/spice-streaming-agent/Makefile.am | 2 +-
>>> include/spice-streaming-agent/errors.hpp | 55 ++++++++++++++++++++++++++++++
>>> src/Makefile.am | 1 +
>>> src/errors.cpp | 56 +++++++++++++++++++++++++++++++
>>> src/spice-streaming-agent.cpp | 5 +++
>>> src/unittests/Makefile.am | 1 +
>>> 6 files changed, 119 insertions(+), 1 deletion(-)
>>> create mode 100644 include/spice-streaming-agent/errors.hpp
>>> create mode 100644 src/errors.cpp
>>>
>>> diff --git a/include/spice-streaming-agent/Makefile.am b/include/spice-streaming-agent/Makefile.am
>>> index 844f791..a223d43 100644
>>> --- a/include/spice-streaming-agent/Makefile.am
>>> +++ b/include/spice-streaming-agent/Makefile.am
>>> @@ -3,5 +3,5 @@ public_includedir = $(includedir)/spice-streaming-agent
>>> public_include_HEADERS = \
>>> frame-capture.hpp \
>>> plugin.hpp \
>>> + errors.hpp \
>>> $(NULL)
>>> -
>>> diff --git a/include/spice-streaming-agent/errors.hpp b/include/spice-streaming-agent/errors.hpp
>>> new file mode 100644
>>> index 0000000..ca70d2e
>>> --- /dev/null
>>> +++ b/include/spice-streaming-agent/errors.hpp
>>> @@ -0,0 +1,55 @@
>>> +/* Errors that may be thrown by the agent
>>> + *
>>> + * \copyright
>>> + * Copyright 2018 Red Hat Inc. All rights reserved.
>>> + */
>>> +#ifndef SPICE_STREAMING_AGENT_ERRORS_HPP
>>> +#define SPICE_STREAMING_AGENT_ERRORS_HPP
>>> +
>>> +#include <exception>
>>> +#include <stddef.h>
>>> +
>>> +namespace spice {
>>> +namespace streaming_agent {
>>> +
>>> +class Error : public std::exception
>>> +{
>>> +public:
>>> + Error(const char *message): exception(), message(message) { }
>>
>> No need to call exception() explicitely. Space before colon?
>>
>>> + Error(const Error &error): exception(error), message(error.message) {}
>>
>> Isn't the generated copy constructor the same?
>>
>>> + virtual const char *what() const noexcept override;
>>> + virtual int format_message(char *buffer, size_t size) const noexcept;
>>> + const Error &syslog() const noexcept;
>>> +protected:
>>> + const char *message;
>>> +};
>>> +
>>> +class IOError : public Error
>>> +{
>>> +public:
>>> + IOError(const char *msg, int saved_errno) : Error(msg), saved_errno(saved_errno) {}
>>> + int format_message(char *buffer, size_t size) const noexcept override;
>>> + int append_strerror(char *buffer, size_t size, int written) const noexcept;
>>
>> append_strerror probably should be protected?
>>
>>> +protected:
>>> + int saved_errno;
>>> +};
>>> +
>>> +class WriteError : public IOError
>>> +{
>>> +public:
>>> + WriteError(const char *msg, const char *operation, int saved_errno)
>>> + : IOError(msg, saved_errno), operation(operation) {}
>>> + int format_message(char *buffer, size_t size) const noexcept override;
>>> +protected:
>>> + const char *operation;
>>
>> I find "operation" a misleading name, you later pass strings like
>> "frame" or "cursor pixels" to it. So:
>>
>> payload_name
>> payload_description
>> data_name
>> data_description
>>
>> Something along those lines?
>>
>>> +};
>>> +
>>> +class ReadError : public IOError
>>> +{
>>> +public:
>>> + ReadError(const char *msg, int saved_errno): IOError(msg, saved_errno) {}
>>> +};
>>> +
>>> +}} // namespace spice::streaming_agent
>>> +
>>> +#endif // SPICE_STREAMING_AGENT_ERRORS_HPP
>>> diff --git a/src/Makefile.am b/src/Makefile.am
>>> index 3717b5c..2507844 100644
>>> --- a/src/Makefile.am
>>> +++ b/src/Makefile.am
>>> @@ -55,4 +55,5 @@ spice_streaming_agent_SOURCES = \
>>> mjpeg-fallback.hpp \
>>> jpeg.cpp \
>>> jpeg.hpp \
>>> + errors.cpp \
>>
>> Frediano pointed out to me you need to add the header file here too so
>> that it gets included in the tarball when you `make dist`.
>
> I've completely missed that you put the header into the public headers,
> so disregard this...
>
> On the public headers, some (atm. most, if not all) errors are meant to
> be private to the agent, right? So we should probably split the errors
> into a public and private header?
No. IMO, a plugin may encounter practically all of them.
>
>>> $(NULL)
>>> diff --git a/src/errors.cpp b/src/errors.cpp
>>> new file mode 100644
>>> index 0000000..01bb162
>>> --- /dev/null
>>> +++ b/src/errors.cpp
>>> @@ -0,0 +1,56 @@
>>> +/* Errors that may be thrown by the agent
>>> + *
>>> + * \copyright
>>> + * Copyright 2018 Red Hat Inc. All rights reserved.
>>> + */
>>> +
>>> +#include <spice-streaming-agent/errors.hpp>
>>> +
>>> +#include <stdio.h>
>>> +#include <syslog.h>
>>> +#include <string.h>
>>> +
>>> +namespace spice {
>>> +namespace streaming_agent {
>>> +
>>> +const char *Error::what() const noexcept
>>> +{
>>> + return message;
>>> +}
>>> +
>>> +int Error::format_message(char *buffer, size_t size) const noexcept
>>> +{
>>> + return snprintf(buffer, size, "%s", message);
>>> +}
>>> +
>>> +const Error &Error::syslog() const noexcept
>>> +{
>>> + char buffer[256];
>>> + format_message(buffer, sizeof(buffer));
>>> + ::syslog(LOG_ERR, "%s\n", buffer);
>>> + return *this;
>>> +}
>>
>> This is actually pretty neat, I like it! Log and throw in one line :)
>>
>>> +
>>> +int IOError::format_message(char *buffer, size_t size) const noexcept
>>> +{
>>> + int written = snprintf(buffer, size, "%s", what());
>>> + return append_strerror(buffer, size, written);
>>> +}
>>> +
>>> +int IOError::append_strerror(char *buffer, size_t size, int written) const noexcept
>>> +{
>>> + // The conversion of written to size_t also deals with the case where written < 0
>>> + if (saved_errno && (size_t) written < size) {
>>> + written += snprintf(buffer + written, size - written, ": %s (errno %d)",
>>> + strerror(saved_errno), saved_errno);
>>> + }
>>> + return written;
>>> +}
>>
>> I have to say I find this a bit too involved just to handle the out of
>> memory state (which, as Christophe F pointed out, most probably means
>> we're going down anyway and don't care much if there was a write
>> error?). And I don't want to reimplement variants of this for the other
>> exception classes we are bound to introduce if we want to stick with
>> this concept...
>>
>> Cheers,
>> Lukas
>>
>>> +
>>> +int WriteError::format_message(char *buffer, size_t size) const noexcept
>>> +{
>>> + int written = snprintf(buffer, size, "%s writing %s", what(), operation);
>>> + return append_strerror(buffer, size, written);
>>> +}
>>> +
>>> +}} // namespace spice::streaming_agent
>>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>>> index 8494a8b..23ee824 100644
>>> --- a/src/spice-streaming-agent.cpp
>>> +++ b/src/spice-streaming-agent.cpp
>>> @@ -13,6 +13,7 @@
>>>
>>> #include <spice-streaming-agent/frame-capture.hpp>
>>> #include <spice-streaming-agent/plugin.hpp>
>>> +#include <spice-streaming-agent/errors.hpp>
>>>
>>> #include <stdio.h>
>>> #include <stdlib.h>
>>> @@ -597,6 +598,10 @@ int main(int argc, char* argv[])
>>> try {
>>> do_capture(stream, streamport, f_log);
>>> }
>>> + catch (Error &err) {
>>> + err.syslog();
>>> + ret = EXIT_FAILURE;
>>> + }
>>> catch (std::runtime_error &err) {
>>> syslog(LOG_ERR, "%s\n", err.what());
>>> ret = EXIT_FAILURE;
>>> diff --git a/src/unittests/Makefile.am b/src/unittests/Makefile.am
>>> index 0fc6d50..ce81b56 100644
>>> --- a/src/unittests/Makefile.am
>>> +++ b/src/unittests/Makefile.am
>>> @@ -39,6 +39,7 @@ test_mjpeg_fallback_SOURCES = \
>>> test-mjpeg-fallback.cpp \
>>> ../jpeg.cpp \
>>> ../mjpeg-fallback.cpp \
>>> + ../errors.cpp \
>>> $(NULL)
>>>
>>> test_mjpeg_fallback_LDADD = \
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel