[libvirt] [PATCH 1/6] util: file: introduce VIR_AUTOCLOSE macro to close fd of the file automatically

[Shi Lei]

On 2018-09-11 at 20:22, Michal Privoznik wrote:
>On 09/10/2018 05:47 AM, Shi Lei wrote:
>> By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE macro,
>> many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to
>> getting rid of many of our cleanup sections.
>>    
>> Signed-off-by: Shi Lei <shi_lei at massclouds.com>
>> ---
>>  src/util/virfile.h | 20 ++++++++++++++++++--
>>  1 file changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/util/virfile.h b/src/util/virfile.h
>> index b30a1d3..70e7203 100644
>> --- a/src/util/virfile.h
>> +++ b/src/util/virfile.h
>> @@ -54,6 +54,11 @@ int virFileClose(int *fdptr, virFileCloseFlags flags)
>>  int virFileFclose(FILE **file, bool preserve_errno) ATTRIBUTE_RETURN_CHECK;
>>  FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK;
>> 
>> +static inline void virForceCloseHelper(int *_fd)
>
>No need for this argument to have underscore in its name. 

Okay. 

>
>> +{
>> +    ignore_value(virFileClose(_fd, VIR_FILE_CLOSE_PRESERVE_ERRNO));
>> +}
>> +
>>  /* For use on normal paths; caller must check return value,
>>     and failure sets errno per close. */
>>  # define VIR_CLOSE(FD) virFileClose(&(FD), 0)
>> @@ -64,8 +69,7 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK;
>> 
>>  /* For use on cleanup paths; errno is unaffected by close,
>>     and no return value to worry about. */
>> -# define VIR_FORCE_CLOSE(FD) \
>> -    ignore_value(virFileClose(&(FD), VIR_FILE_CLOSE_PRESERVE_ERRNO))
>> +# define VIR_FORCE_CLOSE(FD) virForceCloseHelper(&(FD))
>>  # define VIR_FORCE_FCLOSE(FILE) ignore_value(virFileFclose(&(FILE), true))
>> 
>>  /* Similar VIR_FORCE_CLOSE() but ignores EBADF errors since they are expected
>> @@ -80,6 +84,18 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK;
>>                   VIR_FILE_CLOSE_PRESERVE_ERRNO | \
>>                   VIR_FILE_CLOSE_DONT_LOG))
>> 
>> +/**
>> + * VIR_AUTOCLOSE:
>> + * @fd: fd of the file to be closed automatically
>> + *
>> + * Macro to automatically force close the fd by calling virForceCloseHelper
>> + * when the fd goes out of scope. It's used to eliminate VIR_FORCE_CLOSE
>> + * in cleanup sections.
>> + */
>> +# define VIR_AUTOCLOSE(fd) \
>> +    __attribute__((cleanup(virForceCloseHelper))) int fd = -1
>
>While this may helps us to initialize variables correctly, I think we
>should do that explicitly. Not only it follows what VIR_AUTOFREE is
>doing, it also is more visible when used. For instance, in 2/6 when the
>macro is used for the first time, it's not visible what is @fd
>initialized to. 

Okay. So I think the macro could be like:

# define VIR_AUTOCLOSE \
   __attribute__((cleanup(virForceCloseHelper))) int

And the statement is like:

VIR_AUTOCLOSE fd = -1;
VIR_AUTOCLOSE sock_fd = socket(VIR_NETDEV_FAMILY, SOCK_DGRAM, 0);

Also, I think I should add a new syntax-check rule to ensure the initialization
of the variable. Just like sc_require_attribute_cleanup_initialization
for VIR_AUTO(FREE|PTR).

>
>> +
>> +
>>  /* Opaque type for managing a wrapper around a fd.  */
>>  struct _virFileWrapperFd;
>> 
>>
>
>Otherwise the rest of the series looks good.
>
>Michal 

Thanks for your comments.

Shi Lei