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?
> > $(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