[libvirt] RFC: Use attribute ((cleanup) in libvirt ?

Tue Jan 10 13:17:00 UTC 2017

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 at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170110/d1d6d982/attachment-0001.sig>

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

[libvirt] RFC: Use attribute ((cleanup) in libvirt ?