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

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(a)massclouds.com&gt; >> --- >>  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).