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 8db93d987..3ba3aef48 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 4da427789..fc7e506d9 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 000000000..8c3634c40 --- /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 f86546556..6eb3c026d 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 3fe759e8f..2b492a634 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 22a22915d..6460d00f3 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 428158932..0d2767df4 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.39.5