[libvirt] RFC: Use __attribute__ ((cleanup) in libvirt ?
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. As an example of what it would involve... This commit enables the basic helper macros in libguestfs: https://github.com/libguestfs/libguestfs/commit/98b64650c852ccc9a8eef8b96910... These commits make use of them https://github.com/libguestfs/libguestfs/commit/61162bdce1a00a921a47eb3e788e... https://github.com/libguestfs/libguestfs/commit/5a3da366268825b26b470cde3565... https://github.com/libguestfs/libguestfs/commit/791ad3e9e600ef528e3e5a8d50be... Finally, I'm absolutely *not* volunteering to actually implement this idea myself, as I don't have the free time. I just want to raise it as a discussion item, and if we agree its something we'd do, then we can make it a GSoC idea, or let any other interested person hack on it at will. There's no need for a "big bang" convert everything approach, we can do it incrementally. 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/ :|
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-L... https://github.com/libguestfs/libguestfs/blob/master/src/launch-libvirt.c#L7... https://github.com/libguestfs/libguestfs/blob/master/src/appliance-kcmdline.... https://github.com/libguestfs/libguestfs/blob/master/src/filearch.c#L131-L17... 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). Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
On 01/09/2017 08:09 PM, Richard W.M. Jones 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
On Mon, Jan 09, 2017 at 04:58:49PM +0000, Daniel P. Berrange wrote: 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-L... https://github.com/libguestfs/libguestfs/blob/master/src/launch-libvirt.c#L7... https://github.com/libguestfs/libguestfs/blob/master/src/appliance-kcmdline.... https://github.com/libguestfs/libguestfs/blob/master/src/filearch.c#L131-L17...
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
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.
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.
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-L... https://github.com/libguestfs/libguestfs/blob/master/src/launch-libvirt.c#L7... https://github.com/libguestfs/libguestfs/blob/master/src/appliance-kcmdline.... https://github.com/libguestfs/libguestfs/blob/master/src/filearch.c#L131-L17...
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.
You can go around that like this: func() { char *ret = NULL; CLEANUP_FREE char *tmp = NULL; ... if (error) return ret; // or NULL rather ... ret = tmp; tmp = NULL; return ret; }
(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.
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.
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
I'm still afraid it will cause more problems than cleanups, but on the other hand, I like the possibility that compiler features give us. So I can't decide. I'm with Laine on this one. Ambivalent as well. Martin
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.
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.
(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.
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; 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. 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/ :|
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jan 10, 2017 at 02:17:00PM +0100, Martin Kletzander wrote:
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.
Why not? I mean, the GSoC project doesn't need to be for just 1 student, if we're granted the slots you could pick multiple students who would work on it in parallel. Also, there are always means how we could keep track of it, an idea that first crossed my mind without giving any more thinking to it is that you can track the modules still waiting to be switched to the new convention within the GSoC section of our page. The 'virsh auto-completion' project has been in the 'ongoing' state for at least 2 years so I personally don't see an issue here, since it's a bigger task. Erik
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jan 10, 2017 at 02:50:40PM +0100, Erik Skultety wrote:
On Tue, Jan 10, 2017 at 02:17:00PM +0100, Martin Kletzander wrote:
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.
Why not? I mean, the GSoC project doesn't need to be for just 1 student, if we're granted the slots you could pick multiple students who would work on it in parallel. Also, there are always means how we could keep track of it, an idea
You need: 1) To be accepted as an organization 2) Slots 3) Students that are interested in rewriting cleanup all over the code. You cannot depend on any of these.
that first crossed my mind without giving any more thinking to it is that you can track the modules still waiting to be switched to the new convention within the GSoC section of our page. The 'virsh auto-completion' project has been in the 'ongoing' state for at least 2 years so I personally don't see an issue here, since it's a bigger task.
It's also been mentioned by some developers as "really messy" and it's not something that changes how people read code in the whole codebase. Martin
On Tue, Jan 10, 2017 at 02:17:00PM +0100, Martin Kletzander wrote:
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)).
Yeah, it'd be easy to write a rule to manadate NULL initialization for all pointers vars with cleanup attached. Personally, I'm in favour of explicit initialization of every single stack variable no matter whether it happens to be required by the current code structure or not. 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/ :|
On Tue, Jan 10, 2017 at 01:55:02PM +0000, Daniel P. Berrange wrote:
On Tue, Jan 10, 2017 at 02:17:00PM +0100, Martin Kletzander wrote:
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)).
Yeah, it'd be easy to write a rule to manadate NULL initialization for all pointers vars with cleanup attached.
Personally, I'm in favour of explicit initialization of every single stack variable no matter whether it happens to be required by the current code structure or not.
Yes, ^^^this would be absolutely great. Erik
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jan 10, 2017 at 10:00:31AM +0000, Daniel P. Berrange wrote:
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;
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 it's not quite as simple as that because GCC passes the pointer to the pointer. libguestfs uses: #define CLEANUP_FREE __attribute__((cleanup(guestfs_int_cleanup_free))) ... void guestfs_int_cleanup_free (void *ptr) { free (* (void **) ptr); } Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On Tue, Jan 10, 2017 at 11:54:19PM +0000, Richard W.M. Jones wrote:
On Tue, Jan 10, 2017 at 10:00:31AM +0000, Daniel P. Berrange wrote:
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;
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 it's not quite as simple as that because GCC passes the pointer to the pointer. libguestfs uses:
#define CLEANUP_FREE __attribute__((cleanup(guestfs_int_cleanup_free)))
...
void guestfs_int_cleanup_free (void *ptr) { free (* (void **) ptr); }
Well, it is, since we already have that as virFree(), it's just usually called using VIR_FREE() so that you don't have to add an extra ampersand.
Rich.
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/09/2017 05:58 PM, Daniel P. Berrange wrote:
<snip/> I'm wondering what people think of making use of this in libvirt ?
Apart from what have been said (I share the same concerns) I like the idea. All the "limitations" - we already have them. You have to initialize variables even when using 'goto cleanup'. But with this feature we can save few lines of code in a lot of functions. Therefore I'm up for it. BTW: do you know when was this introduced? Whether this feature is available on all prehistoric^Wstill supported systems like RHEL6. BTW: I've added this idea onto our GSoC page: http://wiki.libvirt.org/page/Google_Summer_of_Code_2017#Automatic_freeing_of... Sounds like perfect job for a student to me. Unless there's some deadline to meet. Michal
On Tue, Jan 10, 2017 at 05:05:21PM +0100, Michal Privoznik wrote:
On 01/09/2017 05:58 PM, Daniel P. Berrange wrote:
<snip/> I'm wondering what people think of making use of this in libvirt ?
Apart from what have been said (I share the same concerns) I like the idea. All the "limitations" - we already have them. You have to initialize variables even when using 'goto cleanup'. But with this feature we can save few lines of code in a lot of functions. Therefore I'm up for it. BTW: do you know when was this introduced? Whether this feature is available on all prehistoric^Wstill supported systems like RHEL6.
The docs are non-existant on when this appeared, but I just checked out gcc 4.1.2 and found plenty of references to attribute cleanup. This covers RHEL-5 vintage, so it looks fine. 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/ :|
On 01/09/2017 09:58 AM, 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.
With distro/maintenance hat on, another -.5 is (potentially) more conflicts when backporting upstream fixes to older branches/releases. I do prefer the simplified pattern in this proposal, but like others am on the fence. Regards, Jim
participants (7)
-
Daniel P. Berrange -
Erik Skultety -
Jim Fehlig -
Laine Stump -
Martin Kletzander -
Michal Privoznik -
Richard W.M. Jones