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