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

[Christophe de Dinechin <cdupontd@xxxxxxxxxx>]

On 8 Nov 2017, at 16:35, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:

On 8 Nov 2017, at 16:02, Frediano Ziglio <fziglio@xxxxxxxxxx> 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@xxxxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel