[1/3] libhandle: Remove libattr dependency

Message ID	20240827115032.406321-2-cem@kernel.org (mailing list archive)
State	New
Headers	show Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 58ED7158D9C for <linux-xfs@vger.kernel.org>; Tue, 27 Aug 2024 11:50:40 +0000 (UTC) From: cem@kernel.org To: linux-xfs@vger.kernel.org Cc: djwong@kernel.org, hch@lst.de, hch@infradead.org Subject: [PATCH 1/3] libhandle: Remove libattr dependency Date: Tue, 27 Aug 2024 13:50:22 +0200 Message-ID: <20240827115032.406321-2-cem@kernel.org> In-Reply-To: <20240827115032.406321-1-cem@kernel.org> References: <20240827115032.406321-1-cem@kernel.org> Precedence: bulk MIME-Version: 1.0 Content-Transfer-Encoding: 8bit
Series	Get rid of libattr dependency \| expand [0/3] Get rid of libattr dependency [1/3] libhandle: Remove libattr dependency [2/3] libfrog: remove libattr dependency [3/3] scrub: Remove libattr dependency

Carlos Maiolino Aug. 27, 2024, 11:50 a.m. UTC

From: Carlos Maiolino <cem@kernel.org>

Aiming torwards removing xfsprogs libattr dependency, getting rid of
libattr_cursor is the first step.

libxfs already has our own definition of xfs_libattr_cursor, so we can
simply use it.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 include/handle.h   |  3 +--
 include/jdm.h      |  5 ++---
 libfrog/fsprops.c  |  8 ++++----
 libhandle/handle.c |  2 +-
 libhandle/jdm.c    | 14 +++++++-------
 scrub/phase5.c     |  2 +-
 6 files changed, 16 insertions(+), 18 deletions(-)

Christoph Hellwig Aug. 27, 2024, 12:12 p.m. UTC | #1

On Tue, Aug 27, 2024 at 01:50:22PM +0200, cem@kernel.org wrote:
> +	struct xfs_attrlist_cursor	cur = { };
> +	char				attrbuf[XFS_XATTR_LIST_MAX];
> +	struct attrlist			*attrlist = (struct attrlist *)attrbuf;

Not really changed by this patch, but XFS_XATTR_LIST_MAX feels pretty
large for an on-stack allocation.  Maybe this should use a dynamic
allocation, which would also remove the need for the cast?

Same in few other spots.

Not really something to worry about for this patch, so:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Darrick J. Wong Aug. 27, 2024, 2:41 p.m. UTC | #2

On Tue, Aug 27, 2024 at 01:50:22PM +0200, cem@kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
> 
> Aiming torwards removing xfsprogs libattr dependency, getting rid of
> libattr_cursor is the first step.
> 
> libxfs already has our own definition of xfs_libattr_cursor, so we can
> simply use it.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  include/handle.h   |  3 +--
>  include/jdm.h      |  5 ++---
>  libfrog/fsprops.c  |  8 ++++----
>  libhandle/handle.c |  2 +-
>  libhandle/jdm.c    | 14 +++++++-------
>  scrub/phase5.c     |  2 +-
>  6 files changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/include/handle.h b/include/handle.h
> index ba0650051..991bd5d01 100644
> --- a/include/handle.h
> +++ b/include/handle.h
> @@ -11,7 +11,6 @@ extern "C" {
>  #endif
>  
>  struct fsdmidata;
> -struct attrlist_cursor;
>  struct parent;
>  
>  extern int  path_to_handle (char *__path, void **__hanp, size_t *__hlen);
> @@ -29,7 +28,7 @@ extern int  attr_multi_by_handle (void *__hanp, size_t __hlen, void *__buf,
>  				  int __rtrvcnt, int __flags);
>  extern int  attr_list_by_handle (void *__hanp, size_t __hlen, void *__buf,
>  				 size_t __bufsize, int __flags,
> -				 struct attrlist_cursor *__cursor);
> +				 struct xfs_attrlist_cursor *__cursor);
>  extern int  parents_by_handle(void *__hanp, size_t __hlen,
>  			      struct parent *__buf, size_t __bufsize,
>  			      unsigned int *__count);
> diff --git a/include/jdm.h b/include/jdm.h
> index 50c2296b4..669ee75ce 100644
> --- a/include/jdm.h
> +++ b/include/jdm.h
> @@ -12,7 +12,6 @@ typedef void	jdm_filehandle_t;	/* filehandle */
>  
>  struct xfs_bstat;
>  struct xfs_bulkstat;
> -struct attrlist_cursor;
>  struct parent;
>  
>  extern jdm_fshandle_t *
> @@ -60,11 +59,11 @@ extern intgen_t
>  jdm_attr_list(	jdm_fshandle_t *fshp,
>  		struct xfs_bstat *statp,
>  		char *bufp, size_t bufsz, int flags,
> -		struct attrlist_cursor *cursor);
> +		struct xfs_attrlist_cursor *cursor);
>  
>  intgen_t jdm_attr_list_v5(jdm_fshandle_t *fshp, struct xfs_bulkstat *statp,
>  		char *bufp, size_t bufsz, int flags,
> -		struct attrlist_cursor *cursor);
> +		struct xfs_attrlist_cursor *cursor);

Hmm.  Here you're changing function signatures for a public library.
attrlist_cursor and xfs_attrlist_cursor are the same object, but I
wonder if this is going to cause downstream compilation errors for
programs that include libattr but not xfs_fs.h?

I simply made a shim libfrog/fakelibattr.h that wraps the libhandle
stuff (albeit in a fragile manner, but I don't expect any further
libattr updates) so that we don't have to change libhandle itself.

>  extern int
>  jdm_parents( jdm_fshandle_t *fshp,
> diff --git a/libfrog/fsprops.c b/libfrog/fsprops.c
> index 88046b7a0..05a584a56 100644
> --- a/libfrog/fsprops.c
> +++ b/libfrog/fsprops.c
> @@ -68,10 +68,10 @@ fsprops_walk_names(
>  	fsprops_name_walk_fn	walk_fn,
>  	void			*priv)
>  {
> -	struct attrlist_cursor	cur = { };
> -	char			attrbuf[XFS_XATTR_LIST_MAX];
> -	struct attrlist		*attrlist = (struct attrlist *)attrbuf;
> -	int			ret;
> +	struct xfs_attrlist_cursor	cur = { };
> +	char				attrbuf[XFS_XATTR_LIST_MAX];
> +	struct attrlist			*attrlist = (struct attrlist *)attrbuf;
> +	int				ret;
>  
>  	memset(attrbuf, 0, XFS_XATTR_LIST_MAX);
>  
> diff --git a/libhandle/handle.c b/libhandle/handle.c
> index 1e8fe9ac5..a9a9f9534 100644
> --- a/libhandle/handle.c
> +++ b/libhandle/handle.c
> @@ -381,7 +381,7 @@ attr_list_by_handle(
>  	void		*buf,
>  	size_t		bufsize,
>  	int		flags,
> -	struct attrlist_cursor *cursor)
> +	struct		xfs_attrlist_cursor *cursor)

Odd indenting here.

--D

>  {
>  	int		error, fd;
>  	char		*path;
> diff --git a/libhandle/jdm.c b/libhandle/jdm.c
> index e21aff2b2..9ce605ad3 100644
> --- a/libhandle/jdm.c
> +++ b/libhandle/jdm.c
> @@ -221,7 +221,7 @@ int
>  jdm_attr_list(	jdm_fshandle_t *fshp,
>  		struct xfs_bstat *statp,
>  		char *bufp, size_t bufsz, int flags,
> -		struct attrlist_cursor *cursor)
> +		struct xfs_attrlist_cursor *cursor)
>  {
>  	fshandle_t *fshandlep = ( fshandle_t * )fshp;
>  	filehandle_t filehandle;
> @@ -240,12 +240,12 @@ jdm_attr_list(	jdm_fshandle_t *fshp,
>  
>  int
>  jdm_attr_list_v5(
> -	jdm_fshandle_t		*fshp,
> -	struct xfs_bulkstat	*statp,
> -	char			*bufp,
> -	size_t			bufsz,
> -	int			flags,
> -	struct attrlist_cursor	*cursor)
> +	jdm_fshandle_t			*fshp,
> +	struct xfs_bulkstat		*statp,
> +	char				*bufp,
> +	size_t				bufsz,
> +	int				flags,
> +	struct xfs_attrlist_cursor	*cursor)
>  {
>  	struct fshandle		*fshandlep = (struct fshandle *)fshp;
>  	struct filehandle	filehandle;
> diff --git a/scrub/phase5.c b/scrub/phase5.c
> index f6c295c64..27fa29be6 100644
> --- a/scrub/phase5.c
> +++ b/scrub/phase5.c
> @@ -190,7 +190,7 @@ check_xattr_ns_names(
>  	struct xfs_bulkstat		*bstat,
>  	const struct attrns_decode	*attr_ns)
>  {
> -	struct attrlist_cursor		cur;
> +	struct xfs_attrlist_cursor	cur;
>  	char				attrbuf[XFS_XATTR_LIST_MAX];
>  	char				keybuf[XATTR_NAME_MAX + 1];
>  	struct attrlist			*attrlist = (struct attrlist *)attrbuf;
> -- 
> 2.46.0
> 
>

Darrick J. Wong Aug. 27, 2024, 2:44 p.m. UTC | #3

On Tue, Aug 27, 2024 at 05:12:41AM -0700, Christoph Hellwig wrote:
> On Tue, Aug 27, 2024 at 01:50:22PM +0200, cem@kernel.org wrote:
> > +	struct xfs_attrlist_cursor	cur = { };
> > +	char				attrbuf[XFS_XATTR_LIST_MAX];
> > +	struct attrlist			*attrlist = (struct attrlist *)attrbuf;
> 
> Not really changed by this patch, but XFS_XATTR_LIST_MAX feels pretty
> large for an on-stack allocation.  Maybe this should use a dynamic
> allocation, which would also remove the need for the cast?
> 
> Same in few other spots.

Yeah, the name list buffer here and in scrub could also be reduced
in size to 4k or something like that.  Long ago the attrlist ioctl had a
bug in it where it would loop forever, which is why scrub allocated such
a huge buffer to try to avoid falling into that trap.  But that was
~2017, those kernels should have been retired or patched by now.

--D

> Not really something to worry about for this patch, so:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
>

Christoph Hellwig Aug. 28, 2024, 4:33 a.m. UTC | #4

On Tue, Aug 27, 2024 at 07:44:23AM -0700, Darrick J. Wong wrote:
> On Tue, Aug 27, 2024 at 05:12:41AM -0700, Christoph Hellwig wrote:
> > On Tue, Aug 27, 2024 at 01:50:22PM +0200, cem@kernel.org wrote:
> > > +	struct xfs_attrlist_cursor	cur = { };
> > > +	char				attrbuf[XFS_XATTR_LIST_MAX];
> > > +	struct attrlist			*attrlist = (struct attrlist *)attrbuf;
> > 
> > Not really changed by this patch, but XFS_XATTR_LIST_MAX feels pretty
> > large for an on-stack allocation.  Maybe this should use a dynamic
> > allocation, which would also remove the need for the cast?
> > 
> > Same in few other spots.
> 
> Yeah, the name list buffer here and in scrub could also be reduced
> in size to 4k or something like that.  Long ago the attrlist ioctl had a
> bug in it where it would loop forever, which is why scrub allocated such
> a huge buffer to try to avoid falling into that trap.  But that was
> ~2017, those kernels should have been retired or patched by now.

One nice thing about the dynamic allocation is that we can get rid
of the silly char buffer and the casting..

Christoph Hellwig Aug. 28, 2024, 4:34 a.m. UTC | #5

On Tue, Aug 27, 2024 at 07:41:12AM -0700, Darrick J. Wong wrote:
> Hmm.  Here you're changing function signatures for a public library.
> attrlist_cursor and xfs_attrlist_cursor are the same object, but I
> wonder if this is going to cause downstream compilation errors for
> programs that include libattr but not xfs_fs.h?

Oh, I forgot that jdm.h is public.  Yes, this will cause the compiler
to complain, so we can't do that.

Carlos Maiolino Aug. 29, 2024, 1:18 p.m. UTC | #6

Sorry, came back late for the party...

On Tue, Aug 27, 2024 at 09:34:46PM GMT, Christoph Hellwig wrote:
> On Tue, Aug 27, 2024 at 07:41:12AM -0700, Darrick J. Wong wrote:
> > Hmm.  Here you're changing function signatures for a public library.
> > attrlist_cursor and xfs_attrlist_cursor are the same object, but I
> > wonder if this is going to cause downstream compilation errors for
> > programs that include libattr but not xfs_fs.h?
> 
> Oh, I forgot that jdm.h is public.  Yes, this will cause the compiler
> to complain, so we can't do that.

I spoke with darrick on Tuesday, and it seemed IMO that libhandle is supposed to
be a xfsprogs thing, not something that should be packaged separated, but I
might be totally wrong here.

If we ought to maintain jdm.h intact, then I'd come back to pack definitions
into libfrog/attr.h, but what I don't like about this, is we'll end up with two
definitions for virtually the same thing, xfs_attrlist in libfrog and attrlist
in xfs_fs.h, unless we do use Darrick's idea and mimic libxfs behavior in
libfrog/attr.h, something like:

#define xfs_attrlist_cursor attrlist_cursor

So we could use it within libfrog, without messing up with public libhandle
interface?!

Carlos

[1/3] libhandle: Remove libattr dependency

Commit Message

Comments

Patch