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

[Stefan Berger]

  On 10/15/2010 01:32 PM, Eric Blake wrote:
> On 10/15/2010 10:15 AM, Stefan Berger wrote:
>>
>> Index: libvirt-acl/src/util/files.c
>> ===================================================================
>> --- /dev/null
>> +++ libvirt-acl/src/util/files.c
>
>> + */
>> +
>> +#include <unistd.h>
>
> Oops, need to include <config.h> first.
Ok.
>
>> +++ libvirt-acl/src/util/files.h
>> @@ -0,0 +1,37 @@
>
>> +#ifndef __VIR_FILES_H_
>> +# define __VIR_FILES_H_
>> +
>> +# include "internal.h"
>> +
>> +/* Don't call these directly - use the macros below */
>
> s/these/this/
Ok.
>
>> +int virClose(int *fdptr);
>
> Needs an ATTRIBUTE_NONNULL(1).  I'm also debating whether it needs 
> ATTRIBUTE_RETURN_CHECK; normally, close() fails in very few 
> situations, and in cleanup paths, you tend to already have another 
> error more important so you can ignore the secondary close() 
> failures.  So I'm probably 80-20 against adding ATTRIBUTE_RETURN_CHECK.

Your other mail...
>
>> +
>> +# define VIR_CLOSE(FD) \
>> + virClose(&(FD))
>> +
>> +
>> +#endif /* __VIR_FILES_H */
>> +
>
> No change in src/libvirt_private.syms?
>

Was waiting for the linker to complain and it did not.

>> Index: libvirt-acl/HACKING
>> ===================================================================
>> --- libvirt-acl.orig/HACKING
>> +++ libvirt-acl/HACKING
>
> Yuck - we STILL don't have the auto-generation in place; edits should 
> be to docs/hacking.html.in, and HACKING _should_ be generated from 
> that. Until then, you need to touch both files :(

Ok, will do.
>
>
>> @@ -318,6 +318,17 @@ routines, use the macros from memory.h
>> VIR_FREE(domain);
>>
>>
>> +File handling
>> +=============
>> +
>> +Use of the close() API is deprecated in libvirt code base to help 
>> avoiding
>> +double-closing of a filedescriptor. Instead of this API, use the macro
>> from
>> +files.h
>> +
>> + - eg close a filedescriptor
>
> s/filedescriptor/file descriptor/g
ok.
>
>
>> +
>> + VIR_CLOSE(fd);
>
> Although I'm 80-20 against warning on unused results, we might as well 
> at least make the example demonstrate that the results are valid:
>
> if (VIR_CLOSE(fd) < 0) {
>     virReportSystemError(errno, _("failed to close file"));
> }
>
Replacing the example.

    Stefan