On Fri, 2018-03-02 at 11:53 +0100, Lukáš Hrázký wrote:
> On Fri, 2018-03-02 at 03:03 -0500, Frediano Ziglio wrote:
> > >
> > > > On 1 Mar 2018, at 13:13, Frediano Ziglio <fziglio@xxxxxxxxxx>
> > > > 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.
> > > > >
> > > >
> > > > 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.
>
> This is a very good point by Frediano and I also don't find it
> acceptable, no matter how much you stress it in the docstring. The
> interface is inviting to make this error...
FWIW, I also find myself in agreement with Frediano and Lukas on this
point.
>
> (And as I stated before, I agree with the rest of Frediano's
> reasoning
> on the delayed formatting)
>
> > > 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@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)
> > > >
> > > > 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
> > >
> > >
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel