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@xxxxxxxxxxxxxx>
>> ---
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
