> On 2 Mar 2018, at 11:53, Lukáš Hrázký <lhra...@redhat.com> wrote:
> 
> On Fri, 2018-03-02 at 03:03 -0500, Frediano Ziglio wrote:
>>> 
>>>> On 1 Mar 2018, at 13:13, Frediano Ziglio <fzig...@redhat.com> wrote:
>>>> 
>>>>> 
>>>>> From: Christophe de Dinechin <dinec...@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.
> 
> 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…

See my response to Frediano.

You can’t pass a std::string. You have to explicitly use c_str(). If you do 
that to pass a const char * to an *exception* class, and if you know that the 
passed value will not be alive during unwinding, you are doing something 
extraordinarily stupid.

> 
> (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 <dinec...@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
>>> 
>>> 
>> 
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Reply via email to