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

On 09/12/2018 09:18 AM, Erik Skultety wrote: > On Tue, Sep 11, 2018 at 10:31:18PM +0800, Shi Lei wrote: >> 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 > > Actually, I'd prefer if we stayed consistent with the existing AUTO_ macros, > IOW, the form of VIR_AUTOCLOSE(fd) = <rvalue> is IMHO desired. I don't think this is correct. Sure, we do have parentheses in VIR_AUTOFREE but only for type, not variable itself:

VIR_AUTOFREE(char *) str = NULL; Therefore, in order to be consistent the autoclose macro should look like I'm suggesting actually: VIR_AUTOCLOSE fd = -1; To extend this idea further, we can then have VIR_AUTOFCLOSE and VIR_AUTODIRCLOSE macros to call fclose() and dirclose() respectively (or our wrappers around those functions).