> On 2 Mar 2018, at 09:03, Frediano Ziglio <fziglio@xxxxxxxxxx> 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. The interface is not “prone to errors” at all. The scenario you describe makes no sense, since the whole interface is designed to avoid dynamic strings. This is now explicitlyi documented (see https://github.com/c3d/spice-streaming-agent/blob/c%2B%2B-refactoring/include/spice-streaming-agent/errors.hpp, not yet shared, I’m not entirely done with the reviews). The constructor comment now says: Constructor takes the base message returned by what() The message must remain valid over the whole lifetime of the exception. It is recommended to only use static C strings, since formatting of any dynamic argument is done by 'format_message' Anyway, your example is misleading. Anybody that passes the result of string::c_str() to a const char * without proper consideration for the lifetime is at fault. The const char * interface itself cannot be faulted for that. > n the rare case that on small allocations we are replacing > the error with a std::bad_alloc but introducing other problems. What other problem? If the only one you have is what you just listed, it is not a problem, except for the fool who passes c_str() without consideration for the lifetime of the string. > I prefer to reuse standard classes and support better the not rare cases. You are misusing the standard classes, see C++CG E14 already cited several times: "The standard-library classes derived from exception should be used only as base classes or for exceptions that require only “generic” handling. Like built-in types, their use could clash with other people’s use of them." With an example that shows exactly the problem we have today (what does “runtime_error” mean here?) > >> 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 I don’t understand this sentence at all. Where do I “suppose” anything? As for the accusation of bad design, I take it a bit personally in the current context, because frankly, an error-handling “design" that throws runtime_error for this and that and makes it impossible to catch a write error is a design that IMO sucks. Sorry if you don’t like my trying to fix that. > and also if, as you are saying, the only purpose is getting the message runtime_error is perfectly fine. The assumption in ‘if’ is wrong. It’s not the only purpose. > >> It’s also more efficient, since we only format when we need to, not earlier. >> > > You are always formatting during the exception No. You can catch it to do something with it. Which we can’t with runtime_error, that’s one of the *many* reasons to fix it. > 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. straw man, that’s not my point at all. > Also limit the possible output string and is using plain C function to achieve this this is what I did. > >>> >>>> - 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. I have no idea what you mean with “design dependency”, but no, your solution solves nothing. How would it know which class of exception to format for? How would I do something specific to handle write errors, for example? > >> >>> 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? I am using it. I’m also adding methods that give more detailed information and useful behaviors for errors that we care about. I cannot use “what” for the detailed information without dynamic allocation, which I want to avoid. It’s really hard to understand why you cling so much to std::runtime error with dynamic strings… Using std::runtime_error that way is: 1. Inefficient, generates (at least) one dynamic string allocation and one string concatenation per argument, 2. requires one explicit, manual, boring call to std::to_string for any numerical argument 3. risky, possibly replacing one exception with another (I know the risk is minor, but why do you insist that makes it OK?) 4. blocking the ability to efficiently catch a specific kind of error (i.e. do something specific on a write error that is different from what you’d do on a read error) 5. impeding localization (with my approach, localization including reordering of arguments is possible, snprintf supports it, concatenation does not, see https://daleslocblog.blogspot.fr/2011/10/concatenation-is-evil.html for details) 6. less readable, as “throw WriteError(…)” describes the intent much better than “throw std::runtime_error(“…”); 7. against documented good practices 8. harder to maintain 9. not providing guidance for how to deal with a specific category of errors In all your replies, you have not even begun addressing half of these points, instead handwaving a weak straw-man argument against #3. I find it disappointing. > To support rare cases and adding problems and code? Straw-man. Only the “out of memory while building the string” case is rare. Catching a specific exception class should not be rare. It is only rare in the current code because we painted ourselves in a corner by misusing std::runtime_error. Having to manually call std::to_string is not rare either. And it’s ugly. > >> >>> >>>> 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. Wow. Sorry, but throwing “quality” accusations this way should be done with a little more care. I am addressing what I see as really *serious* quality issues in the *curent* code, ranging from design to safety to … I have been very specific with the many *real* quality problems I have been addressing in this series, which include: - Race conditions - Lack of proper resource cleanup - Inability to deal with specific error conditions - Runtime inefficiency - Incorrect reaction to signals Again, these are *real* quality issues. You have only so far presented a *purely theoretical* case, which I’d describe as a voluntary misuse of the design by passing a string::c_str() to a const char * and blaming the lack of consideration for the lifetime of the result of c_str() on the interface. Sorry, you can do better. > >>> >>>> - >>>> 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. Yeah, like “const char *” as input to an *exception* class is an open invitation to pass a dynamic value which will be destroyed during unwinding. Seriously? So no, it really does not, but I agree this needs to be made explicit in the documentation. See https://github.com/c3d/spice-streaming-agent/blob/c%2B%2B-refactoring/include/spice-streaming-agent/errors.hpp for where I am presently. > 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. I’ve shown enough examples showing how it works, with dynamic parameters that include file names, error numbers, integer values read from the protocol, etc. It’s really not complicated. >>> 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? It makes it possible to make them visible. Although our settings are presently bogus. Christophe F addressed part of it by removing the -fvisibility=hidden from the a.out where it made little sense. But we will also need to add -export-dynamic. And I know that with -export-dynamic, it works: ddd@f25-turbo[c++-refactoring] streaming-agent> DISPLAY=:1 spice-streaming-agent -c invalid=0 spice-streaming-agent[24438]: Error parsing plugin option: invalid MJPEG option name spice-streaming-agent[24438]: exception thrown for testing 'invalid' > a.out? Windows? Windows was the next question. I answered it separately. > >> >>> 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. You wrote “these stuff should be in a DLL”. I don’t know what “these stuff” meant. If it’s the error classes, then I don’t see how or why it would help, Putting the errors in a DLL by themselves made so little sense that I skipped that side of the answer. So I thought you meant the whole set of classes, i.e. the agent, could be put in a DLL. The only scenario where I could see that happening was if we move it to the server side, something we discussed, so that is what I addressed. > >>> >>>> +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 Sorry, all I saw so far is one very weak argument, and a lot of angry handwaving. > >>> >>>> + 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