On 01/09/2017 08:09 PM, Richard W.M. Jones wrote:
On Mon, Jan 09, 2017 at 04:58:49PM +0000, Daniel P. Berrange wrote:
For those who don't already know, GCC and CLang both implement a C language
extension that enables automatic free'ing of resources when variables go
out of scope. This is done by annotating the variable with the "cleanup"
attribute, pointing to a function the compiler will wire up a call to when
unwinding the stack. Since the annotation points to an arbitrary user
defined function, you're not limited to simple free() like semantics. The
cleanup function could unlock a mutex, or decrement a reference count, etc
This annotation is used extensively by systemd, and libguestfs, amongst
other projects. This obviously doesn't bring full garbage collection to
C, but it does enable the code to be simplified. By removing the need to
put in many free() (or equiv) calls to cleanup state, the "interesting"
logic in the code stands out more, not being obscured by cleanup calls
and goto jumps.
I'm wondering what people think of making use of this in libvirt ?
To my mind the only real reason to *not* use it, would be to maintain
code portability to non-GCC/non-CLang compilers. OS-X, *BSD and *Linux
all use GCC or CLang or both, so its a non-issue there. So the only place
this could cause pain is people building libvirt on Win32, who are using
the Microsoft compilers instead og GCC.
IMHO, it is perfectly valid for us to declare that MSVC is unsupported
with Libvirt and users must use GCC to build on Windows, either natively
via cygwin, or cross-build from Linux hosts.
>From the libguestfs POV it's absolutely been a great thing. Of course
we also don't care about MSVC, but that is a possible concern for
libvirt as you say. I wonder if MSVC offers some non-standard support
for this? As it's really a C++ compiler, maybe it's possible to use
that?
There are many many examples of how this simplifies code in
libguestfs. Even very complex functions can often be written with no
"exit path" at all. I'll just point to a few:
https://github.com/libguestfs/libguestfs/blob/master/src/inspect-fs.c#L343-L368
https://github.com/libguestfs/libguestfs/blob/master/src/launch-libvirt.c#L724-L815
https://github.com/libguestfs/libguestfs/blob/master/src/appliance-kcmdline.c#L98
https://github.com/libguestfs/libguestfs/blob/master/src/filearch.c#L131-L179
BTW you can use it for a lot more than just free(). We also have
macros for deleting temporary files and cleaning up complex objects.
Now it's also worth saying there are some catches:
(1) You must not CLEANUP_FREE any objects which you will return from
the function. This often means you still need to explicitly free such
objects on error return paths.
(2) You must not write code like:
fn ()
{
CLEANUP_FREE char *v; // uninitialized
if (some error condition) {
return -1;
}
...
}
because that will call free (v) on the uninitialized variable.
Sometimes GCC can spot this. In libguestfs we tend to initialize
every CLEANUP_* variable to either an explicit value or NULL. GCC
optimizes away calls to free (NULL).
You've covered one of the worries that I had about it (accidentally
marking for CLEANUP a pointer whose value gets returned, and the fact
that you can't use it for the cleanup of objects that would have
normally been returned, in the case that the function encounters an
error and has to dump everything). And since the nice cleanup isn't
happening for *everything*, people will have to be paying attention to
which objects are auto-cleaned up and which aren't, which will
inevitably lead to incorrect classification and/or accidentally adding
manual cleanup for something that's auto-cleaned or vice versa. (and
merging this into the code bit by bit is going to exacerbate this
problem). Also, there is something to be said for having all the code
that's executed sitting out there in the open in an easy to follow
format rather than obscured behind a macro and a compiler directive that
points you up to somewhere else.
So I guess it does sound like a way to make the code look cleaner in
many cases, since it will eliminate a lot of virBlahFree() calls at the
cleanup: label of a function, but it can't eliminate all cleanup code,
so the label itself will likely still be there (or at least an error:
label). I'm skeptical about it making maintenance any simpler overall or
preventing bugs, because it will be one more thing that people have to
pay attention to and potentially get wrong.
My final vote: ambivalent +1, -1
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
