[Spice-devel] [PATCH spice-streaming-server 1/3] Use defer style destruction instead of old C style and goto
Christophe de Dinechin
dinechin at redhat.com
Thu Nov 9 16:56:08 UTC 2017
> On 9 Nov 2017, at 17:54, Frediano Ziglio <fziglio at redhat.com> wrote:
>
> On 9 Nov 2017, at 15:39, Frediano Ziglio <fziglio at redhat.com <mailto: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 <mailto: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 <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/ <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 <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 :-)
The one with zero length?
>
> 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 <mailto:Spice-devel at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
>
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org <mailto:Spice-devel at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/spice-devel <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/b0bd3b1e/attachment-0001.html>
More information about the Spice-devel
mailing list