[v1.1] libfrog: wrap handle construction code

Commit Message

Darrick J. Wong Feb. 20, 2025, 9:36 p.m. UTC

From: Darrick J. Wong <[e-mail]>

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" <[e-mail]>
---
v1.1: explain why this isn't in libhandle for now
---
 libfrog/handle_priv.h |   55 +++++++++++++++++++++++++++++++++++++++++++++++++
 io/parent.c           |    9 +++-----
 libfrog/Makefile      |    1 +
 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

Patch

diff --git a/libfrog/handle_priv.h b/libfrog/handle_priv.h
new file mode 100644
index 00000000000000..8c3634c40de1c8
--- /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 <[e-mail]>
+ */
+#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/io/parent.c b/io/parent.c
index 8db93d98755289..3ba3aef48cb9be 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 4da427789411a6..fc7e506d96bbad 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/scrub/common.c b/scrub/common.c
index f86546556f46dd..6eb3c026dc5ac9 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 3fe759e8f4867d..2b492a634ea3b2 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 @@  scan_ag_bulkstat(
 			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 22a22915dbc68d..6460d00f30f4bd 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 4281589324cd44..0d2767df424f27 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);