Re: [libvirt] RFC: Use __attribute__ ((cleanup) in libvirt ?

On Tue, Jan 10, 2017 at 10:00:31AM +0000, Daniel P. Berrange wrote: > On Tue, Jan 10, 2017 at 10:48:47AM +0100, Martin Kletzander wrote: > > On Tue, Jan 10, 2017 at 02:58:21AM -0500, Laine Stump wrote: > > > 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. > > > > > > > > > Only reason I see for not using it is the "temporary" mess it will > > cause. Yes, we can change to that incrementally, but it will take some > > time and effort and it will never be all of the code that uses it. > > Don't get me wrong, I would love using more builtin compiler features > > and shortening the code here and there. I'm just worried this > > particular one might be more disrupting than useful. Most of us are > > pretty used to the code flow we already have and there's nothing you > > can't achieve without the cleanup attribute. > > > > And yes, I used quotation marks around the word temporary intentionally. > > Yes, that's why I thought of it as something that would make for a GSoc > project - have someone do a full conversion of particular areas of code. > eg convert all of util/ or convert the domain XML parser, etc. Basically > if we did it, I think we'd want to have entire files converted at once. > Only converting individual methods ad-hoc would be quite messy. > Yes, I know, but that still doesn't mean all will be converted, unfortunately.

> > > > > 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. > > > > I would love to know if anyone actually tried doing that lately. Given > > how often we are broken with mingw and we only foind out thanks to our > > test suite (and sometomes the fixing takes more than a release cycle), I > > think nobody does that and from what I know, it might not even work. > > We have mingw in the CI system for a while now and its generally fixed > as quickly as native arch builds are fixed these days. > Yes. Now. But there was a build-breaker for several months that nobody cared about. Pity the builds are truncated so I can't track it back properly. My point is that I don't remember anyone asking about it during the whole time, just us trying to come up with fixes. > > > > > (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). > > > > > > > I'm trying to initialize all variables, always, so I don't see this as a > > problem, but there are some of us that (I have the feeling) are trying > > to initialize as few as possible, so this (although it's a different > > story) might still be a problem for someone. > > We pretty much have the same problem already with 'goto cleanup' - you > have to make sure everything is initialized sanely before the first > "goto cleanup". So I think we're safe in this respect already and > the cleanup attributes wouldn't make it any more complex. > Yeah, but with __attribute__((cleanup)), you need to make sure everything is properly initialized immediatelly as opposed to before first cleanup. I know it sounds easy, and it is. And I love doing that even without __attribute__((cleanup)), I just see the potential for error. Hopefully we'd be able to do a syntax-check rule for checking uninitialized variables with __attribute__((cleanup)). > > > 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. > > > > > > > I don't really like our macros around __attribute__ although I > > understand we need to have some of them to be dynamically defined to > > nothing in some cases. However with __attribute__((cleanup)), we will > > need to have that all the time. What's even better, you immediatelly > > see what function will be called on the cleanup and you can jump to the > > tag definition more easily. > > If we mandate use of gcc / clang, then we wouldn't need to hide it > behind a macro - we'd be able to use it inline. That said, using a > macro makes it smaller and gives a bit of standardization. eg with > libguestfs style: > > #define CLEANUP_FREE __attribute__((cleanup(free))) > #define CLEANUP_OBJECT_UNREF __attribute__((cleanup(virObjectUnref))) > > CLEANUP_FREE char *str; > CLEANUP_OBJECT_UNREF virDomainPtr dom; > > vs full inline style: > > __attribute__((cleanup(free))) char *str; > __attribute__((cleanup(virObjectUnref))) virDomainPtr dom; > I know, my point was that out of these two, I liked the latter better. > That said I see systemd took a halfway house > > #define _cleanup_(x) __attribute__((cleanup(x))) > > _cleanup(free) char *str; > _cleanup(virObjectUnref) virDomainPtr dom; > > > I think the systemd style is quite reasonable, as its shorter > and the function called is still clear. > Yes, this middle ground is perfectly reasonable, readable and tags-searchable. > Regards, > Daniel > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :| > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list

-- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list