[Spice-devel] [PATCH 12/22] Add exception handling classes

[Lukáš Hrázký]

On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin 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.
> 
> - 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 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)
> -
> 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`.

>  	$(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 = \