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

[Frediano Ziglio]

> 
> 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.
> 

Yes, more readable than original.

> 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.
> 

I consider a downside, if I have for instance 2 C open calls and want
to define 2 defers I cannot.

But back to the license "issue" (I don't see it considering the license)
somebody could consider your suggestion inspired by the previous ones
so subject to OpenStack licensing too.

> 
> >
> > 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__);

Frediano