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

[Frediano Ziglio]

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

> > +#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