[Spice-devel] [PATCH 12/22] Add exception handling classes
Christophe de Dinechin
christophe.de.dinechin at gmail.com
Thu Mar 1 18:46:15 UTC 2018
> On 1 Mar 2018, at 13:13, Frediano Ziglio <fziglio at redhat.com> wrote:
>
>>
>> From: Christophe de Dinechin <dinechin at 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.
>>
>
> why not formatting the message constructing the object so before the throw?
That requires dynamic allocation, which is entirely unnecessary and potentially dangerous (replacing one exception kind with another).
You can still format the message at throw point for the purpose of sending it to the syslog. This only uses the stack, no dynamic allocation.
It’s also more efficient, since we only format when we need to, not earlier.
>
>> - a 'syslog' member function that sends the formatted message to the syslog.
>>
>
> why this is needed?
1. DRY principle, avoids repeated (and dispersed) calls to syslog.
2. Along with Error constructor, offers a convenient breakpoint
3. Put all our syslog-for-errors stuff in a single place, easier to change.
> can't just call what() as a standard exception handling?
The standard what() is still supported, returns a base message.
>
>> 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 at 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)
>
> public header? So you are using it on the plugins ?
Not yet, but want to.
>
>> -
>> 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) { }
>
> what if the message came from a std::string::c_str() ?
There is no need to do that when you can pass arguments and do deferred formatting.
> Potentially message will point to an invalid memory location during the format_message/what.
Yes. Don’t do that. Added it to the documentation.
>
>> + Error(const Error &error): exception(error), message(error.message) {}
>> + 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;
>> +};
>> +
>
> As a public header these definition require more documentation.
Added.
> Did you consider visibility?
Yes. The classes are defined in a a.out.
> Did you consider Windows platform? These stuff should be in a DLL.
No, they are in the .exe. Even if we move the agent server side, following Marc-André’s presentation, I’m inclined to think that this should remain a separate process. Too dangerous to bring arbitrary proprietary plugins in the QEMU process space.
>
>> +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;
>> +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;
>> +};
>> +
>> +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 \
>> $(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];
>
> I would use 1024 (standard syslog limit).
Good point. Could not find any obvious SYSLOG_MAX value, though.
> Still I don't think syslog call should be here.
You gave no argument against (“I don’t think” being an opinion, not an argument).
I have given several arguments in favor of this approach, please make sure you address them.
>
>> + format_message(buffer, sizeof(buffer));
>> + ::syslog(LOG_ERR, "%s\n", buffer);
>
> "\n" is not necessary
Removed.
>
>> + return *this;
>> +}
>> +
>> +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;
>> +}
>> +
>> +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 = \
>
> Frediano
More information about the Spice-devel
mailing list