[Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header
Christophe de Dinechin
cdupontd at redhat.com
Tue Feb 20 15:08:38 UTC 2018
> On 20 Feb 2018, at 15:57, Lukáš Hrázký <lhrazky at redhat.com> wrote:
>
> On Tue, 2018-02-20 at 15:48 +0100, Christophe de Dinechin wrote:
>>> 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.
>
> But the exception you suggested has quite a specific constructor
> arguments. We'd eventually need to generalize that, and whatnot... :)
I was just giving an example. But if we look at recent additions in MjpegPlugin::ParseOptions:
try {
settings.fps = stoi(value);
} catch (const std::exception &e) {
throw std::runtime_error("Invalid value '" + value + "' for option 'framerate’.”);
This is repeated twice. Was factored out in my options patch series BTW, and without using exceptions.
Now, it might be useful for some unit test to catch that exception and extract value and option, no?
So that suggests a OptionValueError with message, option and value.
>
>>>
>>> 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.
>
> But you are not handling an exception, you are still constructing one.
> It hasn't been thrown yet.
You are correct, of course. I realized the thinko the moment I hit “send”. Have already replied to myself ;-)
>
>>>
>>> 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
More information about the Spice-devel
mailing list