From 13af03943627dbf44b20b2ae314f5c702375b2cb Mon Sep 17 00:00:00 2001 From: "Darrick J. Wong" Date: Mon, 24 Feb 2025 10:21:41 -0800 Subject: [PATCH] libfrog: wrap handle construction code Clean up all the open-coded logic to construct a file handle from a fshandle and some bulkstat/parent pointer information. The new functions are stashed in a private header file to avoid leaking the details of xfs_handle construction in the public libhandle headers. I tried moving the code to libhandle, but I don't entirely like the result. The libhandle functions pass around handles as arbitrary binary blobs that come from and are sent to the kernel, meaning that the interface is full of (void *, size_t) tuples. Putting these new functions in libhandle breaks that abstraction because now clients know that they can deal with a struct xfs_handle. We could fix that leak by changing it to a (void *, size_t) tuple, but then we'd have to validate the size_t or returns -1 having set errno, which then means that all the client code now has to have error handling for a case that we're fairly sure can't be true. This is overkill for xfsprogs code that knows better, because we can trust ourselves to know the exact layout of a handle. So this nice compact code: memcpy(&handle.ha_fsid, file->fshandle, file->fshandle_len); handle.ha_fid.fid_len = sizeof(xfs_fid_t) - sizeof(handle.ha_fid.fid_len); becomes: ret = handle_from_fshandle(&handle, file->fshandle, file->fshandle_len); if (ret) { perror("what?"); return -1; } Which is much more verbose code, and right now it exists to handle an exceptional condition that is not possible. If someone outside of xfsprogs would like this sort of functionality in libhandle I'm all for adding it, but with zero demand from external users, I prefer to keep things simple. Signed-off-by: "Darrick J. Wong" Reviewed-by: Christoph Hellwig --- io/parent.c | 9 +++---- libfrog/Makefile | 1 + libfrog/handle_priv.h | 55 +++++++++++++++++++++++++++++++++++++++++++ scrub/common.c | 9 +++---- scrub/inodes.c | 13 ++++------ scrub/phase5.c | 12 ++++------ spaceman/health.c | 9 +++---- 7 files changed, 73 insertions(+), 35 deletions(-) create mode 100644 libfrog/handle_priv.h diff --git a/io/parent.c b/io/parent.c index 8db93d98..3ba3aef4 100644 --- a/io/parent.c +++ b/io/parent.c @@ -11,6 +11,7 @@ #include "handle.h" #include "init.h" #include "io.h" +#include "libfrog/handle_priv.h" static cmdinfo_t parent_cmd; static char *mntpt; @@ -205,12 +206,8 @@ parent_f( return 0; } - memcpy(&handle, hanp, sizeof(handle)); - handle.ha_fid.fid_len = sizeof(xfs_fid_t) - - sizeof(handle.ha_fid.fid_len); - handle.ha_fid.fid_pad = 0; - handle.ha_fid.fid_ino = ino; - handle.ha_fid.fid_gen = gen; + handle_from_fshandle(&handle, hanp, hlen); + handle_from_inogen(&handle, ino, gen); } else if (optind != argc) { return command_usage(&parent_cmd); } diff --git a/libfrog/Makefile b/libfrog/Makefile index 4da42778..fc7e506d 100644 --- a/libfrog/Makefile +++ b/libfrog/Makefile @@ -53,6 +53,7 @@ fsgeom.h \ fsproperties.h \ fsprops.h \ getparents.h \ +handle_priv.h \ histogram.h \ logging.h \ paths.h \ diff --git a/libfrog/handle_priv.h b/libfrog/handle_priv.h new file mode 100644 index 00000000..8c3634c4 --- /dev/null +++ b/libfrog/handle_priv.h @@ -0,0 +1,55 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2025 Oracle. All Rights Reserved. + * Author: Darrick J. Wong + */ +#ifndef __LIBFROG_HANDLE_PRIV_H__ +#define __LIBFROG_HANDLE_PRIV_H__ + +/* + * Private helpers to construct an xfs_handle without publishing those details + * in the public libhandle header files. + */ + +/* + * Fills out the fsid part of a handle. This does not initialize the fid part + * of the handle; use either of the two functions below. + */ +static inline void +handle_from_fshandle( + struct xfs_handle *handle, + const void *fshandle, + size_t fshandle_len) +{ + ASSERT(fshandle_len == sizeof(xfs_fsid_t)); + + memcpy(&handle->ha_fsid, fshandle, sizeof(handle->ha_fsid)); + handle->ha_fid.fid_len = sizeof(xfs_fid_t) - + sizeof(handle->ha_fid.fid_len); + handle->ha_fid.fid_pad = 0; + handle->ha_fid.fid_ino = 0; + handle->ha_fid.fid_gen = 0; +} + +/* Fill out the fid part of a handle from raw components. */ +static inline void +handle_from_inogen( + struct xfs_handle *handle, + uint64_t ino, + uint32_t gen) +{ + handle->ha_fid.fid_ino = ino; + handle->ha_fid.fid_gen = gen; +} + +/* Fill out the fid part of a handle. */ +static inline void +handle_from_bulkstat( + struct xfs_handle *handle, + const struct xfs_bulkstat *bstat) +{ + handle->ha_fid.fid_ino = bstat->bs_ino; + handle->ha_fid.fid_gen = bstat->bs_gen; +} + +#endif /* __LIBFROG_HANDLE_PRIV_H__ */ diff --git a/scrub/common.c b/scrub/common.c index f8654655..6eb3c026 100644 --- a/scrub/common.c +++ b/scrub/common.c @@ -10,6 +10,7 @@ #include "platform_defs.h" #include "libfrog/paths.h" #include "libfrog/getparents.h" +#include "libfrog/handle_priv.h" #include "xfs_scrub.h" #include "common.h" #include "progress.h" @@ -414,12 +415,8 @@ scrub_render_ino_descr( if (ctx->mnt.fsgeom.flags & XFS_FSOP_GEOM_FLAGS_PARENT) { struct xfs_handle handle; - memcpy(&handle.ha_fsid, ctx->fshandle, sizeof(handle.ha_fsid)); - handle.ha_fid.fid_len = sizeof(xfs_fid_t) - - sizeof(handle.ha_fid.fid_len); - handle.ha_fid.fid_pad = 0; - handle.ha_fid.fid_ino = ino; - handle.ha_fid.fid_gen = gen; + handle_from_fshandle(&handle, ctx->fshandle, ctx->fshandle_len); + handle_from_inogen(&handle, ino, gen); ret = handle_to_path(&handle, sizeof(struct xfs_handle), 4096, buf, buflen); diff --git a/scrub/inodes.c b/scrub/inodes.c index 3fe759e8..2b492a63 100644 --- a/scrub/inodes.c +++ b/scrub/inodes.c @@ -19,6 +19,7 @@ #include "descr.h" #include "libfrog/fsgeom.h" #include "libfrog/bulkstat.h" +#include "libfrog/handle_priv.h" /* * Iterate a range of inodes. @@ -209,7 +210,7 @@ scan_ag_bulkstat( xfs_agnumber_t agno, void *arg) { - struct xfs_handle handle = { }; + struct xfs_handle handle; struct scrub_ctx *ctx = (struct scrub_ctx *)wq->wq_ctx; struct scan_ichunk *ichunk = arg; struct xfs_inumbers_req *ireq = ichunk_to_inumbers(ichunk); @@ -225,12 +226,7 @@ scan_ag_bulkstat( DEFINE_DESCR(dsc_inumbers, ctx, render_inumbers_from_agno); descr_set(&dsc_inumbers, &agno); - - memcpy(&handle.ha_fsid, ctx->fshandle, sizeof(handle.ha_fsid)); - handle.ha_fid.fid_len = sizeof(xfs_fid_t) - - sizeof(handle.ha_fid.fid_len); - handle.ha_fid.fid_pad = 0; - + handle_from_fshandle(&handle, ctx->fshandle, ctx->fshandle_len); retry: bulkstat_for_inumbers(ctx, &dsc_inumbers, inumbers, breq); @@ -244,8 +240,7 @@ retry: continue; descr_set(&dsc_bulkstat, bs); - handle.ha_fid.fid_ino = scan_ino; - handle.ha_fid.fid_gen = bs->bs_gen; + handle_from_bulkstat(&handle, bs); error = si->fn(ctx, &handle, bs, si->arg); switch (error) { case 0: diff --git a/scrub/phase5.c b/scrub/phase5.c index 22a22915..6460d00f 100644 --- a/scrub/phase5.c +++ b/scrub/phase5.c @@ -18,6 +18,7 @@ #include "libfrog/bitmap.h" #include "libfrog/bulkstat.h" #include "libfrog/fakelibattr.h" +#include "libfrog/handle_priv.h" #include "xfs_scrub.h" #include "common.h" #include "inodes.h" @@ -474,9 +475,7 @@ retry_deferred_inode( if (error) return error; - handle->ha_fid.fid_ino = bstat.bs_ino; - handle->ha_fid.fid_gen = bstat.bs_gen; - + handle_from_bulkstat(handle, &bstat); return check_inode_names(ncs->ctx, handle, &bstat, ncs); } @@ -487,16 +486,13 @@ retry_deferred_inode_range( uint64_t len, void *arg) { - struct xfs_handle handle = { }; + struct xfs_handle handle; struct ncheck_state *ncs = arg; struct scrub_ctx *ctx = ncs->ctx; uint64_t i; int error; - memcpy(&handle.ha_fsid, ctx->fshandle, sizeof(handle.ha_fsid)); - handle.ha_fid.fid_len = sizeof(xfs_fid_t) - - sizeof(handle.ha_fid.fid_len); - handle.ha_fid.fid_pad = 0; + handle_from_fshandle(&handle, ctx->fshandle, ctx->fshandle_len); for (i = 0; i < len; i++) { error = retry_deferred_inode(ncs, &handle, ino + i); diff --git a/spaceman/health.c b/spaceman/health.c index 42815893..0d2767df 100644 --- a/spaceman/health.c +++ b/spaceman/health.c @@ -14,6 +14,7 @@ #include "libfrog/bulkstat.h" #include "space.h" #include "libfrog/getparents.h" +#include "libfrog/handle_priv.h" static cmdinfo_t health_cmd; static unsigned long long reported; @@ -317,12 +318,8 @@ report_inode( (file->xfd.fsgeom.flags & XFS_FSOP_GEOM_FLAGS_PARENT)) { struct xfs_handle handle; - memcpy(&handle.ha_fsid, file->fshandle, sizeof(handle.ha_fsid)); - handle.ha_fid.fid_len = sizeof(xfs_fid_t) - - sizeof(handle.ha_fid.fid_len); - handle.ha_fid.fid_pad = 0; - handle.ha_fid.fid_ino = bs->bs_ino; - handle.ha_fid.fid_gen = bs->bs_gen; + handle_from_fshandle(&handle, file->fshandle, file->fshandle_len); + handle_from_bulkstat(&handle, bs); ret = handle_to_path(&handle, sizeof(struct xfs_handle), 0, descr, sizeof(descr) - 1); -- 2.47.2