On 9 Nov 2017, at 15:39, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
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@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
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.You can. You just need to put a surrounding block to make the lifetime ofeach of your cleanups more explicit.My argument is really whether this is not dangerous. I think it could beencouraging bad code like:fd = open(…)defer { close(fd); };close(fd);// Copy-paste from the above, with variationsfd = open(…)defer { close(fd); }kaboom();close(fd);This code is bad as in: it is broken on the infrequently taken exception path.Now we have two defer objects, and if something bad happens in kaboom(),we end up closing fd twice.
in the normal case you end up closing the file 3 times, not just 2.
See the issue with multiple defer in a single block and why I’d rather avoid it?
still convinced is better to allow multiple defer.
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.There is no such thing as a license on ideas. And as I pointed out, thatspecific idea did not even originate where you found it. Being “inspired by”does not make code derivative. Linux is a prime example, being verystrongly inspired by Unix down to system call names, but not derived from it.You happen to have found a “defer” macro based on the Go syntax. Butif you came from a Java background, finally has been around for a while.
C++ introduced exceptions with finally in C++90, Java was published in 1995.
But the syntax is quite different requiring blocks and having the cleanup part far
from allocation.
I’ve seen several variants of macro-based cleanup code that did not evenneed templates or lambdas, and worked pretty much the same, stuff like that:#define micro_defer(Code) micro_defer_(Code,__COUNTER__)#define micro_defer_(Code,N) micro_defer__(Code,N)#define micro_defer__(Code,N) struct Defer##N { ~Defer##N() { Code } } defer##N
Usage: micro_defer(fclose(fd)); Only syntactic difference is using () instead
no, you cannot capture or use local variables, this code won't even compile, or at
least with many compilers.
__COUNTER__ is better in this case compared to __LINE__, just is not standard,
we can replace __LINE__ with __COUNTER__.
of { }. Not a big deal. Much simpler to understand, easier for the compiler toprocess. That stuff has been around for so long I have no idea who to givecredit to, assuming it’s not me.If you were coming from an iOS background, you’d use that kind of thing ona semi-regular basis. For example, one that emulates the “defer” keyword in straightBy the way, its macro drudgery uses __COUNTER__ instead of __LINE__…
I won't call that straight C. __COUNTER__ is an extension, different compilers implements
it. The cleanup attribute is a GCC extension and is one of the few attributes that are not just
an hint. And the block extension, implemented only by Clang (and as a patch in GCC), looks
like a partial implementation of lambda.
If we start considering which license we should apply to every snippet ofcode explaining a well-known, if clever, C or C++ trick, we are not out of the woods.There are tons of ways to achieve the same objective, with minor syntacticvariations.So quite frankly, let’s call that a clever programming technique, becausethat’s what it is. And then let’s rewrite our own keeping in mind that we wantto write production code and not unreadable snippets, and refer to the on-linediscussion(s) as an explanation for how it works.
My legal knowledge are not that great.
Honestly I assume that if somebody publish something that came from him is public
if not specified otherwise.
How a chain of different knowledge is passed by is not that easy.
In this case I almost agree with you... still... there's a copyright on /bin/true source
file :-)
Frediano
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
_______________________________________________
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