<html><body><div style="font-family: times new roman, new york, times, serif; font-size: 12pt; color: #000000"><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div><blockquote class=""><div class="">On 9 Nov 2017, at 15:39, Frediano Ziglio <<a href="mailto:fziglio@redhat.com" class="" target="_blank">fziglio@redhat.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div style="" class=""><blockquote style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class=""><br class="">Christophe Fergeau writes:<br class=""><br class=""><blockquote class="">On Wed, Nov 08, 2017 at 03:02:35PM +0000, Frediano Ziglio wrote:<br class=""><blockquote class="">This better integrate with exceptions.<br class="">Also don't leak resources using a return in the middle of the<br class="">code.<br class=""><br class="">Signed-off-by: Frediano Ziglio <<a href="mailto:fziglio@redhat.com" class="" target="_blank">fziglio@redhat.com</a>><br class="">---<br class="">src/Makefile.am | 1 +<br class="">src/defer.hpp | 16 ++++++++++++++++<br class="">src/spice-streaming-agent.cpp | 16 ++++++++--------<br class="">3 files changed, 25 insertions(+), 8 deletions(-)<br class="">create mode 100644 src/defer.hpp<br class=""><br class="">diff --git a/src/Makefile.am b/src/Makefile.am<br class="">index 8d5c5bd..ec1969a 100644<br class="">--- a/src/Makefile.am<br class="">+++ b/src/Makefile.am<br class="">@@ -56,4 +56,5 @@ spice_streaming_agent_SOURCES = \<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>mjpeg-fallback.cpp \<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>jpeg.cpp \<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>jpeg.hpp \<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span>defer.hpp \<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>$(NULL)<br class="">diff --git a/src/defer.hpp b/src/defer.hpp<br class="">new file mode 100644<br class="">index 0000000..93931fe<br class="">--- /dev/null<br class="">+++ b/src/defer.hpp<br class="">@@ -0,0 +1,16 @@<br class="">+// see<br class=""><a href="https://stackoverflow.com/questions/32432450/what-is-standard-defer-finalizer-implementation-in-c" class="" target="_blank">https://stackoverflow.com/questions/32432450/what-is-standard-defer-finalizer-implementation-in-c</a><br class=""></blockquote><br class="">Is there any license associated with that code snippet?<br class=""></blockquote><br class="">That code snippet and variants thereof have been floating around for<br class="">quite a while. Here is a discussion of this topic I found in 2012:<br class=""><br class=""><a href="http://the-witness.net/news/2012/11/scopeexit-in-c11/" class="" target="_blank">http://the-witness.net/news/2012/11/scopeexit-in-c11/</a><br class=""><br class="">In one of the comments, there is something that is really close.<br class=""><br class="">If we want to invent our own by fear of licensing issues, I would go for<br class="">something like this:<br class=""><br class="">struct Defer {<br class=""> template <typename Action> struct Cleanup<br class=""> {<br class=""> ~Cleanup() { action(); }<br class=""> Action action;<br class=""> };<br class=""> template <typename Action><br class=""> friend inline Cleanup<Action> operator >> (Defer, Action action)<br class=""> {<br class=""> return Cleanup<Action> { action };<br class=""> }<br class=""><br class="">};<br class="">#define my_defer auto _cleanup_ = Defer{} >> [&]() -> void<br class=""><br class="">Compared to the original, changes are somewhat minor, but I find them useful:<br class=""><br class="">1. There is no "dummy" class. Whatever is returned is a local<br class="">Defer::Cleanup class. If we ever need to run that code in a debugger, I<br class="">think it will be more explicit where things are coming from.<br class=""><br class="">2. I tried to make the code slightly more "readable", e.g. by naming<br class="">things a bit better, and using operator>> which is more visible in the<br class="">code than operator*.<br class=""><br class="">3. I use friend name injection to make all the definitions "fit" inside<br class="">Defer.<br class=""><br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Yes, more readable than original.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">4. I avoid the macro drudgery. Downside is you have only one defer per<br class="">scope. Upside is you have only one defer per scope. I see that as an<br class="">upside.<br class=""><br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">I consider a downside, if I have for instance 2 C open calls and want</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">to define 2 defers I cannot.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></div></blockquote><div><br class=""></div><div>You can. You just need to put a surrounding block to make the lifetime of</div><div>each of your cleanups more explicit.</div><div><br class=""></div><div>My argument is really whether this is not dangerous. I think it could be</div><div>encouraging bad code like:</div><div><br class=""></div><div>fd = open(…)</div><div>defer { close(fd); };</div><div>close(fd);</div><div><br class=""></div><div>// Copy-paste from the above, with variations</div><div>fd = open(…)</div><div>defer { close(fd); }</div><div>kaboom();</div><div>close(fd);</div><div><br class=""></div><div>This code is bad as in: it is broken on the infrequently taken exception path.</div><div>Now we have two defer objects, and if something bad happens in kaboom(),</div><div>we end up closing fd twice.</div></div></blockquote><div>in the normal case you end up closing the file 3 times, not just 2.<br></div><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div><div>See the issue with multiple defer in a single block and why I’d rather avoid it?</div></div></blockquote><div>still convinced is better to allow multiple defer.<br></div><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div><blockquote class=""><div class=""><div style="" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">But back to the license "issue" (I don't see it considering the license)</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">somebody could consider your suggestion inspired by the previous ones</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">so subject to OpenStack licensing too.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></div></blockquote><div><br class=""></div><div>There is no such thing as a license on ideas. And as I pointed out, that</div><div>specific idea did not even originate where you found it. Being “inspired by”</div><div>does not make code derivative. Linux is a prime example, being very</div><div>strongly inspired by Unix down to system call names, but not derived from it.</div><div><br class=""></div><div>You happen to have found a “defer” macro based on the Go syntax. But</div><div><div>if you came from a Java background, finally has been around for a while.</div></div></div></blockquote><div>C++ introduced exceptions with finally in C++90, Java was published in 1995.<br></div><div>But the syntax is quite different requiring blocks and having the cleanup part far<br></div><div>from allocation.<br></div><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div><div><div>I’ve seen several variants of macro-based cleanup code that did not even</div><div>need templates or lambdas, and worked pretty much the same, stuff like that:</div><div><br class=""></div><div><div>#define micro_defer(Code) micro_defer_(Code,__COUNTER__)</div><div>#define micro_defer_(Code,N) micro_defer__(Code,N)</div><div>#define micro_defer__(Code,N) struct Defer##N { ~Defer##N() { Code } } defer##N</div></div></div></div></blockquote><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div><div><div><div><div>Usage: micro_defer(fclose(fd)); Only syntactic difference is using () instead</div></div></div></div></div></blockquote><div>no, you cannot capture or use local variables, this code won't even compile, or at<br></div><div>least with many compilers.<br></div><div>__COUNTER__ is better in this case compared to __LINE__, just is not standard,<br></div><div>we can replace __LINE__ with __COUNTER__.<br></div><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div><div><div><div><div class="">of { }. Not a big deal. Much simpler to understand, easier for the compiler to</div><div class="">process. That stuff has been around for so long I have no idea who to give</div><div class="">credit to, assuming it’s not me.</div></div><div><br class=""></div></div><div>If you were coming from an iOS background, you’d use that kind of thing on</div><div>a semi-regular basis. For example, one that emulates the “defer” keyword in straight</div><div>C here: <a href="http://fdiv.net/2015/10/08/emulating-defer-c-clang-or-gccblocks" class="" target="_blank">http://fdiv.net/2015/10/08/emulating-defer-c-clang-or-gccblocks</a>.</div><div>By the way, its macro drudgery uses __COUNTER__ instead of __LINE__…</div></div></div></blockquote><div>I won't call that straight C. __COUNTER__ is an extension, different compilers implements<br></div><div>it. The cleanup attribute is a GCC extension and is one of the few attributes that are not just</div><div>an hint. And the block extension, implemented only by Clang (and as a patch in GCC), looks</div><div> like a partial implementation of lambda.<br></div><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div><div><div>If we start considering which license we should apply to every snippet of</div><div>code explaining a well-known, if clever, C or C++ trick, we are not out of the woods.</div><div>There are tons of ways to achieve the same objective, with minor syntactic</div><div>variations.</div><div><br class=""></div><div>So quite frankly, let’s call that a clever programming technique, because</div><div>that’s what it is. And then let’s rewrite our own keeping in mind that we want</div><div>to write production code and not unreadable snippets, and refer to the on-line</div><div>discussion(s) as an explanation for how it works.</div></div></div></blockquote><div>My legal knowledge are not that great.<br></div><div>Honestly I assume that if somebody publish something that came from him is public<br></div><div>if not specified otherwise.<br></div><div><br></div><div>How a chain of different knowledge is passed by is not that easy.<br></div><div>In this case I almost agree with you... still... there's a copyright on /bin/true source</div><div>file :-)<br></div><div><br></div><div>Frediano<br></div><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div><blockquote class=""><div class=""><div style="" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class=""><br class=""><blockquote class=""><br class="">Christophe<br class=""><br class=""><blockquote class="">+#ifndef defer<br class="">+template <class F> struct deferrer<br class="">+{<br class="">+ F f;<br class="">+ ~deferrer() { f(); }<br class="">+};<br class="">+struct defer_dummy {};<br class="">+template <class F> inline deferrer<F> operator*(defer_dummy, F f)<br class="">+{<br class="">+ return {f};<br class="">+}<br class="">+#define DFRCAT_(LINE) _defer##LINE<br class="">+#define DFRCAT(LINE) DFRCAT_(LINE)<br class="">+#define defer auto DFRCAT(__LINE__) = defer_dummy{} *[&]() -> void<br class="">+#endif<br class="">diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp<br class="">index ed7ddb9..4122eee 100644<br class="">--- a/src/spice-streaming-agent.cpp<br class="">+++ b/src/spice-streaming-agent.cpp<br class="">@@ -33,6 +33,7 @@<br class=""><br class="">#include "hexdump.h"<br class="">#include "concrete-agent.hpp"<br class="">+#include "defer.hpp"<br class=""><br class="">using namespace std;<br class="">using namespace SpiceStreamingAgent;<br class="">@@ -353,12 +354,17 @@ do_capture(const char *streamport, FILE *f_log)<br class=""> // TODO was syslog(LOG_ERR, "Failed to open %s: %s\n",<br class=""> streamport, strerror(errno));<br class=""> throw std::runtime_error("failed to open streaming device");<br class=""><br class="">+ defer {<br class="">+ close(streamfd);<br class="">+ streamfd = -1;<br class="">+ };<br class="">+<br class=""> unsigned int frame_count = 0;<br class=""> while (! quit) {<br class=""> while (!quit && !streaming_requested) {<br class=""> if (read_command(1) < 0) {<br class=""> syslog(LOG_ERR, "FAILED to read command\n");<br class="">- goto done;<br class="">+ return;<br class=""> }<br class=""> }<br class=""><br class="">@@ -409,19 +415,13 @@ do_capture(const char *streamport, FILE *f_log)<br class=""> //usleep(1);<br class=""> if (read_command(0) < 0) {<br class=""> syslog(LOG_ERR, "FAILED to read command\n");<br class="">- goto done;<br class="">+ return;<br class=""> }<br class=""> if (!streaming_requested) {<br class=""> capture->Reset();<br class=""> }<br class=""> }<br class=""> }<br class="">-<br class="">-done:<br class="">- if (streamfd >= 0) {<br class="">- close(streamfd);<br class="">- streamfd = -1;<br class="">- }<br class="">}<br class=""><br class="">#define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);<br class=""></blockquote></blockquote></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Frediano</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">_______________________________________________</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Spice-devel mailing list</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="mailto:Spice-devel@lists.freedesktop.org" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="" target="_blank">Spice-devel@lists.freedesktop.org</a><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="https://lists.freedesktop.org/mailman/listinfo/spice-devel" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="" target="_blank">https://lists.freedesktop.org/mailman/listinfo/spice-devel</a><br data-mce-bogus="1"></div></div></blockquote></div><br class=""></blockquote><div><br></div></div></body></html>