> On 2 Mar 2018, at 17:16, Jonathon Jongsma <jjongsma@xxxxxxxxxx> wrote:
>
> 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.
OK. Maybe I misunderstood something.
Do we agree that the case Frediano raised is someone trying:
throw Error(“The value of “ + std::to_string(2) + “ does not match “ + std::to_string(1));
which yields the following error:
spice-streaming-agent.cpp:165:93: error: no matching function for call to ‘spice::streaming_agent::Error::Error(std::__cxx11::basic_string<char>)’
So far, I think the interface was not “error prone”. It caught the error correctly.
Therefore, in order to find the interface “error prone”, one has to accept a second hypothesis, which is that whoever attempted that, after seeing that error message, looks at the interface, which has a ‘const char *’ and a comment that specifically states "It is recommended to only use static C strings, since formatting of any dynamic argument is done by ‘format_message’”, and after reading that comment, has a big “Aha” moment and writes:
throw Error((“The value of “ + std::to_string(2) + “ does not match “ + std::to_string(1)).c_str());
I’m sorry, but I cannot accept that step of the reasoning as being even remotely valid. Just looking at the code makes me want to barf.
So either I misunderstood Frediano’s point, or the reasoning is deeply flawed in a not very subtle way.
I’ll let it stew over the week-end, I’m tired of arguing what appears as an evidence to me.
Also, it’s not a vote, it’s a matter of Frediano’s reasoning being correct or not.
>
>>
>> (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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel