[Spice-devel] [PATCH spice-streaming-server 1/3] Use defer style destruction instead of old C style and goto

[Christophe de Dinechin]

> On 8 Nov 2017, at 16:35, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
>>> 
>>> On 8 Nov 2017, at 16:02, Frediano Ziglio <fziglio at redhat.com> wrote:
>>> 
>>> This better integrate with exceptions.
>>> Also don't leak resources using a return in the middle of the
>>> code.
>>> 
>>> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>>> ---
>>> src/Makefile.am               |  1 +
>>> src/defer.hpp                 | 16 ++++++++++++++++
>>> src/spice-streaming-agent.cpp | 16 ++++++++--------
>>> 3 files changed, 25 insertions(+), 8 deletions(-)
>>> create mode 100644 src/defer.hpp
>>> 
>>> diff --git a/src/Makefile.am b/src/Makefile.am
>>> index 8d5c5bd..ec1969a 100644
>>> --- a/src/Makefile.am
>>> +++ b/src/Makefile.am
>>> @@ -56,4 +56,5 @@ spice_streaming_agent_SOURCES = \
>>> 	mjpeg-fallback.cpp \
>>> 	jpeg.cpp \
>>> 	jpeg.hpp \
>>> +	defer.hpp \
>>> 	$(NULL)
>>> diff --git a/src/defer.hpp b/src/defer.hpp
>>> new file mode 100644
>>> index 0000000..93931fe
>>> --- /dev/null
>>> +++ b/src/defer.hpp
>>> @@ -0,0 +1,16 @@
>>> +// see
>>> https://stackoverflow.com/questions/32432450/what-is-standard-defer-finalizer-implementation-in-c
>>> +#ifndef defer
>>> +template <class F> struct deferrer
>>> +{
>>> +    F f;
>>> +    ~deferrer() { f(); }
>>> +};
>>> +struct defer_dummy {};
>>> +template <class F> inline deferrer<F> operator*(defer_dummy, F f)
>>> +{
>>> +    return {f};
>>> +}
>>> +#define DFRCAT_(LINE) _defer##LINE
>>> +#define DFRCAT(LINE) DFRCAT_(LINE)
>>> +#define defer auto DFRCAT(__LINE__) = defer_dummy{} *[&]() -> void
>> 
>> I would add a ‘static’ here in the unlikely case someone uses the defer macro
>> twice from the same line in different files.
>> 
> 
> But this would destroy the object at the end of the program, which is
> completely different from what you want.

Argh. I was considering a possible use in global scope, where we could have duplicate definitions, and that made me forget the primary usage in local scope. Duh.

> 
>>> +#endif
>>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>>> index ed7ddb9..4122eee 100644
>>> --- a/src/spice-streaming-agent.cpp
>>> +++ b/src/spice-streaming-agent.cpp
>>> @@ -33,6 +33,7 @@
>>> 
>>> #include "hexdump.h"
>>> #include "concrete-agent.hpp"
>>> +#include "defer.hpp"
>>> 
>>> using namespace std;
>>> using namespace SpiceStreamingAgent;
>>> @@ -353,12 +354,17 @@ do_capture(const char *streamport, FILE *f_log)
>>>        // TODO was syslog(LOG_ERR, "Failed to open %s: %s\n", streamport,
>>>        strerror(errno));
>>>        throw std::runtime_error("failed to open streaming device");
>>> 
>>> +    defer {
>> 
>> Why not keep the old if(streamfd >= 0) test?
>> 
> 
> The test is already done above... but maybe is safer to add in case is
> closed by some other function, I'll do it.
> 
>>> +        close(streamfd);
>>> +        streamfd = -1;
>>> +    };
>>> +
>>>    unsigned int frame_count = 0;
>>>    while (! quit) {
>>>        while (!quit && !streaming_requested) {
>>>            if (read_command(1) < 0) {
>>>                syslog(LOG_ERR, "FAILED to read command\n");
>>> -                goto done;
>>> +                return;
>>>            }
>>>        }
>>> 
>>> @@ -409,19 +415,13 @@ do_capture(const char *streamport, FILE *f_log)
>>>            //usleep(1);
>>>            if (read_command(0) < 0) {
>>>                syslog(LOG_ERR, "FAILED to read command\n");
>>> -                goto done;
>>> +                return;
>>>            }
>>>            if (!streaming_requested) {
>>>                capture->Reset();
>>>            }
>>>        }
>>>    }
>>> -
>>> -done:
>>> -    if (streamfd >= 0) {
>>> -        close(streamfd);
>>> -        streamfd = -1;
>>> -    }
>>> }
>>> 
>>> #define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org <mailto:Spice-devel at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20171108/a5a14e91/attachment-0001.html>