Re: [PATCH 2/2] libxfs: clean up libxfs_destroy

[Brian Foster <bfoster@redhat.com>]

On Mon, Feb 24, 2020 at 04:11:26PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> It's weird that libxfs_init opens the three devices passed in via the
> libxfs_xinit structure but libxfs_destroy doesn't actually close them.
> Fix this inconsistency and remove all the open-coded device closing.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  copy/xfs_copy.c     |    2 +-
>  db/init.c           |    8 +-------
>  include/libxfs.h    |    2 +-
>  libxfs/init.c       |   31 +++++++++++++++++++++++--------
>  mkfs/xfs_mkfs.c     |    7 +------
>  repair/xfs_repair.c |    7 +------
>  6 files changed, 28 insertions(+), 29 deletions(-)
> 
> 
> diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c
> index a6d67038..7f4615ac 100644
> --- a/copy/xfs_copy.c
> +++ b/copy/xfs_copy.c
> @@ -1200,7 +1200,7 @@ main(int argc, char **argv)
>  
>  	check_errors();
>  	libxfs_umount(mp);
> -	libxfs_destroy();
> +	libxfs_destroy(&xargs);
>  
>  	return 0;
>  }
> diff --git a/db/init.c b/db/init.c
> index 0ac37368..e5450d2b 100644
> --- a/db/init.c
> +++ b/db/init.c
> @@ -217,13 +217,7 @@ main(
>  	while (iocur_sp > start_iocur_sp)
>  		pop_cur();
>  	libxfs_umount(mp);
> -	if (x.ddev)
> -		libxfs_device_close(x.ddev);
> -	if (x.logdev && x.logdev != x.ddev)
> -		libxfs_device_close(x.logdev);
> -	if (x.rtdev)
> -		libxfs_device_close(x.rtdev);
> -	libxfs_destroy();
> +	libxfs_destroy(&x);
>  
>  	return exitcode;
>  }
> diff --git a/include/libxfs.h b/include/libxfs.h
> index aaac00f6..504f6e9c 100644
> --- a/include/libxfs.h
> +++ b/include/libxfs.h
> @@ -136,7 +136,7 @@ typedef struct libxfs_xinit {
>  extern char	*progname;
>  extern xfs_lsn_t libxfs_max_lsn;
>  extern int	libxfs_init (libxfs_init_t *);
> -extern void	libxfs_destroy (void);
> +void		libxfs_destroy(struct libxfs_xinit *li);
>  extern int	libxfs_device_to_fd (dev_t);
>  extern dev_t	libxfs_device_open (char *, int, int, int);
>  extern void	libxfs_device_close (dev_t);
> diff --git a/libxfs/init.c b/libxfs/init.c
> index 197690df..913f546f 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -259,6 +259,21 @@ destroy_zones(void)
>  	return leaked;
>  }
>  
> +static void
> +libxfs_close_devices(
> +	struct libxfs_xinit	*li)
> +{
> +	if (li->ddev)
> +		libxfs_device_close(li->ddev);
> +	if (li->logdev && li->logdev != li->ddev)
> +		libxfs_device_close(li->logdev);
> +	if (li->rtdev)
> +		libxfs_device_close(li->rtdev);
> +
> +	li->ddev = li->logdev = li->rtdev = 0;
> +	li->dfd = li->logfd = li->rtfd = -1;
> +}
> +
>  /*
>   * libxfs initialization.
>   * Caller gets a 0 on failure (and we print a message), 1 on success.
> @@ -385,12 +400,9 @@ libxfs_init(libxfs_init_t *a)
>  		unlink(rtpath);
>  	if (fd >= 0)
>  		close(fd);
> -	if (!rval && a->ddev)
> -		libxfs_device_close(a->ddev);
> -	if (!rval && a->logdev)
> -		libxfs_device_close(a->logdev);
> -	if (!rval && a->rtdev)
> -		libxfs_device_close(a->rtdev);
> +	if (!rval)
> +		libxfs_close_devices(a);
> +
>  	return rval;
>  }
>  
> @@ -913,9 +925,12 @@ libxfs_umount(
>   * Release any global resources used by libxfs.
>   */
>  void
> -libxfs_destroy(void)
> +libxfs_destroy(
> +	struct libxfs_xinit	*li)
>  {
> -	int	leaked;
> +	int			leaked;
> +
> +	libxfs_close_devices(li);
>  
>  	/* Free everything from the buffer cache before freeing buffer zone */
>  	libxfs_bcache_purge();
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 1038e604..7f315d8a 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3945,11 +3945,6 @@ main(
>  	if (error)
>  		exit(1);
>  
> -	if (xi.rtdev)
> -		libxfs_device_close(xi.rtdev);
> -	if (xi.logdev && xi.logdev != xi.ddev)
> -		libxfs_device_close(xi.logdev);
> -	libxfs_device_close(xi.ddev);
> -	libxfs_destroy();
> +	libxfs_destroy(&xi);
>  	return 0;
>  }
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index ccb13f4a..38578121 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -1111,12 +1111,7 @@ _("Note - stripe unit (%d) and width (%d) were copied from a backup superblock.\
>  	if (error)
>  		exit(1);
>  
> -	if (x.rtdev)
> -		libxfs_device_close(x.rtdev);
> -	if (x.logdev && x.logdev != x.ddev)
> -		libxfs_device_close(x.logdev);
> -	libxfs_device_close(x.ddev);
> -	libxfs_destroy();
> +	libxfs_destroy(&x);
>  
>  	if (verbose)
>  		summary_report();
>