diff mbox series

[2/2] libxfs: clean up libxfs_destroy

Message ID 158258948621.451256.5275982330161893261.stgit@magnolia (mailing list archive)
State Accepted
Headers show
Series libxfs: minor cleanups of destructors | expand

Commit Message

Darrick J. Wong Feb. 25, 2020, 12:11 a.m. UTC 
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>
---
 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(-)

Comments

Eric Sandeen Feb. 25, 2020, 6 a.m. UTC | #1 
On 2/24/20 4:11 PM, 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.

Seems better.  Part of me wishes the device open/close weren't hidden
in init/destroy but from a quick glance that's harder to tease apart.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>
Brian Foster Feb. 25, 2020, 3:13 p.m. UTC | #2 
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();
>
Christoph Hellwig Feb. 25, 2020, 5:41 p.m. UTC | #3 
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.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
diff mbox series

Patch

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();