[Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header

[Christophe de Dinechin]

> On 20 Feb 2018, at 15:45, Lukáš Hrázký <lhrazky at redhat.com> wrote:
> 
> On Tue, 2018-02-20 at 15:07 +0100, Christophe de Dinechin wrote:
>>> On 19 Feb 2018, at 17:47, Frediano Ziglio <fziglio at redhat.com> wrote:
>>> 
>>>> 
>>>> On Mon, 2018-02-19 at 15:52 +0000, Frediano Ziglio wrote:
>>>>> Allows to reuse it.
>>>>> 
>>>>> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>>>>> ---
>>>>> src/Makefile.am        |  1 +
>>>>> src/mjpeg-fallback.cpp |  7 +------
>>>>> src/utils.hpp          | 18 ++++++++++++++++++
>>>>> 3 files changed, 20 insertions(+), 6 deletions(-)
>>>>> create mode 100644 src/utils.hpp
>>>>> 
>>>>> diff --git a/src/Makefile.am b/src/Makefile.am
>>>>> index 3717b5c..ba3b1bf 100644
>>>>> --- a/src/Makefile.am
>>>>> +++ b/src/Makefile.am
>>>>> @@ -55,4 +55,5 @@ spice_streaming_agent_SOURCES = \
>>>>> 	mjpeg-fallback.hpp \
>>>>> 	jpeg.cpp \
>>>>> 	jpeg.hpp \
>>>>> +	utils.hpp \
>>>>> 	$(NULL)
>>>>> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
>>>>> index cf704c6..0f31834 100644
>>>>> --- a/src/mjpeg-fallback.cpp
>>>>> +++ b/src/mjpeg-fallback.cpp
>>>>> @@ -6,6 +6,7 @@
>>>>> 
>>>>> #include <config.h>
>>>>> #include "mjpeg-fallback.hpp"
>>>>> +#include "utils.hpp"
>>>>> 
>>>>> #include <cstring>
>>>>> #include <exception>
>>>>> @@ -19,12 +20,6 @@
>>>>> 
>>>>> using namespace spice::streaming_agent;
>>>>> 
>>>>> -#define ERROR(args) do { \
>>>>> -    std::ostringstream _s; \
>>>>> -    _s << args; \
>>>>> -    throw std::runtime_error(_s.str()); \
>>>>> -} while(0)
>>>>> -
>>>>> static inline uint64_t get_time()
>>>>> {
>>>>>    timespec now;
>>>>> diff --git a/src/utils.hpp b/src/utils.hpp
>>>>> new file mode 100644
>>>>> index 0000000..1e43eff
>>>>> --- /dev/null
>>>>> +++ b/src/utils.hpp
>>>>> @@ -0,0 +1,18 @@
>>>>> +/* Miscellaneous utilities
>>>>> + *
>>>>> + * \copyright
>>>>> + * Copyright 2018 Red Hat Inc. All rights reserved.
>>>>> + */
>>>>> +#ifndef SPICE_STREAMING_AGENT_UTILS_HPP
>>>>> +#define SPICE_STREAMING_AGENT_UTILS_HPP
>>>>> +
>>>>> +#include <stdexcept>
>>>>> +#include <sstream>
>>>>> +
>>>>> +#define ERROR(args) do { \
>>>>> +    std::ostringstream _s; \
>>>>> +    _s << args; \
>>>>> +    throw std::runtime_error(_s.str()); \
>>>>> +} while(0)
>>>>> +
>>>>> +#endif // SPICE_STREAMING_AGENT_UTILS_HPP
>>>> 
>>>> Can we please not do this :) It isn't such a chore to throw the
>>>> exceptions directly. This adds a level of indirection (code-wise) and
>>>> macros are (personal opinion alert) best avoided in C++ unless you
>>>> absolutely have to...
>>>> 
>>>> Lukas
>>>> 
>>> 
>>> Yes, I'm taking to much shortcuts. I was considering a format library like
>>> https://github.com/fmtlib/fmt. Did you even used a format library?
> 
> Forgot to reply... I haven't used it, and as Christophe, I also
> probably wouldn't, though maybe for different reasons :)
> 
>> No, I would not do that.
>> 
>> When you are about to throw an exception, you should probably avoid anything that may fail in a different way, e.g. run out of memory. This is the reason std::runtime_error::what returns a const char * and not a string.
>> 
>> If you want to do some formatting, may I suggest deferred formatting, which addresses that problem. In other words, instead of:
>> 
>> 	throw std::runtime_error(std::string(“Flobnic error “) + std::to_string(error_id) + “ when doing “ + operation”);
>> 
>> consider:
>> 
>> 	struct flobnic_error : std::runtime_error
>> 	{
>> 		flobnic_error(const char *message, unsigned error_id, const char *operation)
>> 			: std::runtime_error(message), error_id(error_id), operation(operation) {}
>> 		// Optional
>> 		const char *what() { /* format here, but risky */ }
>> 		unsigned error_id;
>> 		const char *operation;
>> 	}
>> 
>> and then whoever catches can then properly filter errors:
>> 
>> 	catch(flobnic_error &f) {
>> 		// Emit message here that can use error_id and operation
>> 		// If you defined your own “what”, you may use it here
>> 		// (and need to decide where the formatted “what” was built
>> 		// i.e. whether you need to free it)
>> 	}
>> 
>> It’s a little more work, but it:
>> - Passes more information around (both exception type and the original arguments
>> - Avoids additional runtime errors that might arise when formatting before throwing.
>> 
>> 
>> Does that make sense?
> 
> I don't think this is practical... For some cases, it does make sense
> to create exception classes, but in general, we have some many
> different errors throughout the code that creating exceptions for each
> of them indeed doesn't make any sense :)

You really only need one class, let’s call it ’SpiceError’, and a discriminating enum.

> 
> How do you propose in general to handle these arbitrary errors, for
> which we are throwing runtime_errors now? You may have noticed I've
> been replacing some old error handling with just what you described :)
> I've also never seen a problem with it, used it a _lot_ :) It would
> obviously manifest probably only when running out of memory... In that
> case, you'd get some random bad_alloc exception one way or the other?

I’d say you should get terminate, since you throw an exception during exception handling.

> 
> Lukas
> 
>>> 
>>> Frediano
>>> _______________________________________________
>>> Spice-devel mailing list
>>> Spice-devel at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>> 
>> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel