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

[Martin Kletzander]

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-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.
>>

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
-------------- 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/4491515c/attachment-0001.sig>