> On 1 Mar 2018, at 12:40, Lukáš Hrázký <lhra...@redhat.com> wrote:
>
> On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinec...@redhat.com>
>>
>> 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 <dinec...@redhat.com>
>> ---
>> 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.
Not indispensable, but useful, as it signals work to be done if ever someone
changes the base class.
> Space before colon?
Yes.
>
>> + Error(const Error &error): exception(error), message(error.message) {}
>
> Isn't the generated copy constructor the same?
Yes. Removed.
>
>> + 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?
Does not hurt.
>
>> +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?
It’s called “operation” in “write_all” as well. It’s not really a payload,
since we may also be writing a header. To me, “write operation” is a better
name than your alternatives.
>
>> +};
>> +
>> +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`.
It’s in include/spice-streaming-agent, because I want it to be usable by
plugins.
>
>> $(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
Knowing that we presently call string::operator+ and std::to_string to patch
together error messages, I find your definition of “involved” pretty 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?).
Straw man. This is not the only reason by far.
> 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…
This is in the base class, so no need to reimplement, see later patches,
ReadError and WriteError as a proof.
>
> 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
