[Spice-devel] [PATCH 12/22] Add exception handling classes
Frediano Ziglio
fziglio at redhat.com
Fri Mar 2 08:03:29 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).
>
As explained by my string.c_str example your interface is prone to
errors. In the rare case that on small allocations we are replacing
the error with a std::bad_alloc but introducing other problems.
I prefer to reuse standard classes and support better the not rare cases.
> 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.
>
Supposing that you only get the error is a bad design and also if, as
you are saying, the only purpose is getting the message runtime_error
is perfectly fine.
> It’s also more efficient, since we only format when we need to, not earlier.
>
You are always formatting during the exception which being "exceptional"
is by definition cold path. If in the event of error you don't log anything
is faster, not a big point.
Also limit the possible output string and is using plain C function to
achieve this
> >
> >> - 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.
>
void report_error(const std::exception& exp); resolves all the above
without introducing design dependencies.
>
> > can't just call what() as a standard exception handling?
>
> The standard what() is still supported, returns a base message.
>
The question is why not using it instead of rewriting it?
To support rare cases and adding problems and code?
>
> >
> >> 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.
>
If this is the quality level I would reject this patch.
This is mostly replacing a current no-problem with multiple
problems.
> >
> >> -
> >> 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.
>
Don't agree, having a base class and passing a "const char*" as a
general message suggests in many cases to have a dynamic message.
If for instance for some cases you want to add line informations
the message will be dynamic. You are basically forcing users to know this
detail and add another class inheriting from Error and using C code
to format the string, is not that flexible just to add some information.
> > 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.
>
Do you think that this make visible from plugins loaded dynamically?
a.out? Windows?
>
> > 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.
>
?? don't apply here. We are talking about running on the guest and
portability design.
> >
> >> +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.
>
See above
> >
> >> + 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