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

Thu Nov 9 16:54:40 UTC 2017

> > On 9 Nov 2017, at 15:39, Frediano Ziglio < fziglio at redhat.com > 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 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.
> 

> You can. You just need to put a surrounding block to make the lifetime of
> each of your cleanups more explicit.

> My argument is really whether this is not dangerous. I think it could be
> encouraging bad code like:

> fd = open(…)
> defer { close(fd); };
> close(fd);

> // Copy-paste from the above, with variations
> fd = 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, that
> specific idea did not even originate where you found it. Being “inspired by”
> does not make code derivative. Linux is a prime example, being very
> strongly 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. But
> if 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 even
> need 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
> to
> process. That stuff has been around for so long I have no idea who to give
> credit to, assuming it’s not me.

> If you were coming from an iOS background, you’d use that kind of thing on
> a semi-regular basis. For example, one that emulates the “defer” keyword in
> straight
> C here: http://fdiv.net/2015/10/08/emulating-defer-c-clang-or-gccblocks .
> By 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 of
> code 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 syntactic
> variations.

> So quite frankly, let’s call that a clever programming technique, because
> that’s what it is. And then let’s rewrite our own keeping in mind that we
> want
> to write production code and not unreadable snippets, and refer to the
> on-line
> discussion(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 at lists.freedesktop.org
> 
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20171109/9600b315/attachment-0001.html>