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

Brian Foster
2020-02-25T15:13:32Z

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