[libvirt] [PATCH] introduce VIR_CLOSE to be used rather than close()

[Laine Stump]

  On 10/15/2010 01:01 PM, Stefan Berger wrote:
>  On 10/15/2010 12:54 PM, Matthias Bolte wrote:
>> 2010/10/15 Stefan Berger<stefanb at linux.vnet.ibm.com>:
>>>   Since bugs due to double-closed filedescriptors are difficult to 
>>> track down
>>> in a multi-threaded system, I am introducing the VIR_CLOSE(fd) macro 
>>> to help
>>> avoid mistakes here.
>>>
>>> There are lots of places where close() is being used. In this patch 
>>> I am
>>> only cleaning up usage of close() in src/conf where the problems were.
>>>
>>> I also dare to declare close() as being deprecated in libvirt code base
>>> (HACKING). :-)
>>>
>>> Signed-off-by: Stefan Berger<stefanb at us.ibm.com>
>>
>>> Index: libvirt-acl/src/conf/domain_conf.c
>>> ===================================================================
>>> --- libvirt-acl.orig/src/conf/domain_conf.c
>>> +++ libvirt-acl/src/conf/domain_conf.c
>>> @@ -46,6 +46,7 @@
>>>   #include "nwfilter_conf.h"
>>>   #include "ignore-value.h"
>>>   #include "storage_file.h"
>>> +#include "files.h"
>>>
>>>   #define VIR_FROM_THIS VIR_FROM_DOMAIN
>>>
>>> @@ -6798,17 +6799,18 @@ int virDomainSaveXML(const char *configD
>>>          goto cleanup;
>>>      }
>>>
>>> -    if (close(fd)<  0) {
>>> +    if (VIR_CLOSE(fd)<  0) {
>>>          virReportSystemError(errno,
>>>                               _("cannot save config file '%s'"),
>>>                               configFile);
>>> -        goto cleanup;
>>> +        goto cleanup_free;
>>>      }
>>>
>>>      ret = 0;
>>>   cleanup:
>>> -    if (fd != -1)
>>> -        close(fd);
>>> +    VIR_CLOSE(fd);
>>> +
>>> + cleanup_free:
>>>      VIR_FREE(configFile);
>>>      return ret;
>>>   }
>> Why did you add a new label here? You build VIR_CLOSE in a way that
>> allows safe double invocation. Therefore, this is just fine, isn't it:
>
> I had it without this new label and then changed it since I think it's 
> still good practice to not try to invoke the function twice, even 
> though nothing can go wrong.
>
>     Stefan

VIR_FREE() is commonly used in the way that Matthias describes, so I 
think it would be consistent to use VIR_CLOSE in this way as well.

Invoking it unconditionally simplifies the code, and eliminates possible 
confusion when someone adds new code and isn't sure which label they 
should goto on failure (or maybe they think they are sure, but make the 
wrong assumption, thus leading to a leaked fd as a result of trying to 
eliminate possible double-closed fd's ;-)