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

[Shi Lei]

On 2018-09-12 at 16:37, Erik Skultety wrote:
>On Wed, Sep 12, 2018 at 10:32:04AM +0200, Michal Privoznik wrote:
>> 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 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
>> >
>> > 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:
>
>Oh, right, I missed that fact, sorry for the noise.
>
>>
>>   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).
>
>I believe the general consensus is to always use our wrappers. I'm thinking
>whether this could be made more generic, but that would just introduce more
>unnecessary black magic, so I'm fine with your suggestion for the time being,
>we might come up with something better eventually.
>
>Erik 

Okay.
So, I would follow the suggestion from both of you and post the v2 series.

Shi Lei