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

[Erik Skultety]

On Wed, Sep 12, 2018 at 04:04:04PM +0800, Shi Lei wrote:
> On 2018-09-12 at 15:18, 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.
>
> Okay. Then the syntax-check rule would be simpler.
>
> >
> >>
> >> 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).
> >
> >Yep, I agree with adding a similar syntax-check rule.
> >
> >Erik
>
> Okay. Just now, I find that we do not need to add a new rule. A minor change on
> sc_require_attribute_cleanup_initialization can work.

Even better.

Regards,
Erik