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

[Christophe de Dinechin]

Christophe Fergeau writes:

> On Wed, Nov 08, 2017 at 03:02:35PM +0000, Frediano Ziglio 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
>
> Is there any license associated with that code snippet?

That code snippet and variants thereof have been floating around for
quite a while. Here is a discussion of this topic I found in 2012:

http://the-witness.net/news/2012/11/scopeexit-in-c11/

In one of the comments, there is something that is really close.

If we want to invent our own by fear of licensing issues, I would go for
something like this:

struct Defer {
    template <typename Action> struct Cleanup
    {
        ~Cleanup() { action(); }
        Action action;
    };
    template <typename Action>
    friend inline Cleanup<Action> operator >> (Defer, Action action)
    {
        return Cleanup<Action> { action };
    }

};
#define my_defer auto _cleanup_ = Defer{} >> [&]() -> void

Compared to the original, changes are somewhat minor, but I find them useful:

1. There is no "dummy" class. Whatever is returned is a local
Defer::Cleanup class. If we ever need to run that code in a debugger, I
think it will be more explicit where things are coming from.

2. I tried to make the code slightly more "readable", e.g. by naming
things a bit better, and using operator>> which is more visible in the
code than operator*.

3. I use friend name injection to make all the definitions "fit" inside
Defer.

4. I avoid the macro drudgery. Downside is you have only one defer per
scope. Upside is you have only one defer per scope. I see that as an
upside.


>
> Christophe
>
>> +#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
>> +#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 {
>> +        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__);
>> --
>> 2.13.6
>>
>> _______________________________________________
>> 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


--
Cheers,
Christophe de Dinechin (IRC c3d)