On Thu, Feb 06, 2025 at 08:49:22PM -0800, Darrick J. Wong wrote:
> On Thu, Feb 06, 2025 at 08:34:36PM -0800, Christoph Hellwig wrote:
> > On Thu, Feb 06, 2025 at 02:31:41PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> > >
> > > 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.
> >
> > So creating a handle from bulkstat is a totally normal thing to do
> > for xfs-aware applications. I'd much rathe see this in libhandle
> > than hiding it away.
>
> I was going to protest that even xfsdump doesn't construct its own weird
> handle, but Debian codesearch says that Ganesha does it, so I'll move it
> to actual libhandle.
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.
For now I'm leaving the declarations as taking a pointer to mutable
struct xfs_handle. This change also causes the libhandle version number
to jump from 1.0.3 to 1.1.0.
--D
> > > + handle_from_fshandle(&handle, file->fshandle, file->fshandle_len);
> >
> > Nit: overly long line.
>
> Will fix.
>
> --D
>
