[PATCH 0/3] Get rid of libattr dependency

[PATCH 0/3] Get rid of libattr dependency

[cem]

From: Carlos Maiolino <cem@kernel.org>

libxfs already defines most of the structs used by xfsprogs, so we don't
really need to link against libattr.

To make things more organized, and smaller easy to review patches, remove it in steps,


This is only compile-tested so far. I just sent the patches for testing,
but giving it might take a while, I'm sending it to the list for review.

Carlos Maiolino (3):
  libhandle: Remove libattr dependency
  libfrog: remove libattr dependency
  scrub: Remove libattr dependency

 include/handle.h   |  3 +--
 include/jdm.h      |  5 ++---
 libfrog/Makefile   |  7 ++-----
 libfrog/attr.h     | 23 +++++++++++++++++++++++
 libfrog/fsprops.c  | 13 ++++++-------
 libhandle/handle.c |  2 +-
 libhandle/jdm.c    | 14 +++++++-------
 scrub/Makefile     |  4 ----
 scrub/phase5.c     | 11 ++++-------
 9 files changed, 46 insertions(+), 36 deletions(-)
 create mode 100644 libfrog/attr.h

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
-- 
2.46.0

[PATCH 1/3] libhandle: Remove libattr dependency

[cem]

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);
 
 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)
 {
 	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

[PATCH 2/3] libfrog: remove libattr dependency

[cem]

From: Carlos Maiolino <cem@kernel.org>

Get rid of libfrog's libattr dependency, and move the needed local
definitions to a new header - libfrog/attr.h

We could keep the ATTR_ENTRY definition local to fsprops.h, but we'll
add more content to it in the next patch.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 libfrog/Makefile  |  7 ++-----
 libfrog/attr.h    | 18 ++++++++++++++++++
 libfrog/fsprops.c |  7 +++----
 3 files changed, 23 insertions(+), 9 deletions(-)
 create mode 100644 libfrog/attr.h

diff --git a/libfrog/Makefile b/libfrog/Makefile
index acddc894e..8581c146b 100644
--- a/libfrog/Makefile
+++ b/libfrog/Makefile
@@ -21,6 +21,7 @@ crc32.c \
 file_exchange.c \
 fsgeom.c \
 fsproperties.c \
+fsprops.c \
 getparents.c \
 histogram.c \
 list_sort.c \
@@ -49,6 +50,7 @@ div64.h \
 file_exchange.h \
 fsgeom.h \
 fsproperties.h \
+fsprops.h \
 getparents.h \
 histogram.h \
 logging.h \
@@ -62,11 +64,6 @@ workqueue.h
 
 LSRCFILES += gen_crc32table.c
 
-ifeq ($(HAVE_LIBATTR),yes)
-CFILES+=fsprops.c
-HFILES+=fsprops.h
-endif
-
 LDIRT = gen_crc32table crc32table.h
 
 default: ltdepend $(LTLIBRARY)
diff --git a/libfrog/attr.h b/libfrog/attr.h
new file mode 100644
index 000000000..9110499f2
--- /dev/null
+++ b/libfrog/attr.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (c) 2024 Red Hat, Inc.  All Rights Reserved.
+ * Author: Carlos Maiolino <cmaiolino@redhat.com>
+ */
+#ifndef __LIBFROG_ATTR_H__
+#define __LIBFROG_ATTR_H__
+
+/*
+ * Those definitions come from libattr
+ *
+ * We are redifining here so we don't need to keep libattr as a dependency anymore
+ */
+#define ATTR_ENTRY(buffer, index)		\
+	((struct xfs_attrlist_ent *)		\
+	 &((char *)buffer)[ ((struct xfs_attrlist *)(buffer))->al_offset[index] ])
+
+#endif /* __LIBFROG_ATTR_H__ */
diff --git a/libfrog/fsprops.c b/libfrog/fsprops.c
index 05a584a56..ea47c66ed 100644
--- a/libfrog/fsprops.c
+++ b/libfrog/fsprops.c
@@ -10,8 +10,7 @@
 #include "libfrog/bulkstat.h"
 #include "libfrog/fsprops.h"
 #include "libfrog/fsproperties.h"
-
-#include <attr/attributes.h>
+#include "libfrog/attr.h"
 
 /*
  * Given an xfd and a mount table path, get us the handle for the root dir so
@@ -70,7 +69,7 @@ fsprops_walk_names(
 {
 	struct xfs_attrlist_cursor	cur = { };
 	char				attrbuf[XFS_XATTR_LIST_MAX];
-	struct attrlist			*attrlist = (struct attrlist *)attrbuf;
+	struct xfs_attrlist		*attrlist = (struct xfs_attrlist *)attrbuf;
 	int				ret;
 
 	memset(attrbuf, 0, XFS_XATTR_LIST_MAX);
@@ -81,7 +80,7 @@ fsprops_walk_names(
 		unsigned int	i;
 
 		for (i = 0; i < attrlist->al_count; i++) {
-			struct attrlist_ent	*ent = ATTR_ENTRY(attrlist, i);
+			struct xfs_attrlist_ent	*ent = ATTR_ENTRY(attrlist, i);
 			const char		*p =
 				attr_name_to_fsprop_name(ent->a_name);
 
-- 
2.46.0

[PATCH 3/3] scrub: Remove libattr dependency

[cem]

From: Carlos Maiolino <cem@kernel.org>

Rename all attrlist usage to xfs_attrlist, and add a couple more
definitions to libfrog/attr.h

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 libfrog/attr.h | 5 +++++
 scrub/Makefile | 4 ----
 scrub/phase5.c | 9 +++------
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/libfrog/attr.h b/libfrog/attr.h
index 9110499f2..f1a10b5ae 100644
--- a/libfrog/attr.h
+++ b/libfrog/attr.h
@@ -11,8 +11,13 @@
  *
  * We are redifining here so we don't need to keep libattr as a dependency anymore
  */
+
 #define ATTR_ENTRY(buffer, index)		\
 	((struct xfs_attrlist_ent *)		\
 	 &((char *)buffer)[ ((struct xfs_attrlist *)(buffer))->al_offset[index] ])
 
+/* Attr flags used within xfsprogs, must match the definitions from libattr */
+#define ATTR_ROOT	0x0002	/* use root namespace attributes in op */
+#define ATTR_SECURE	0x0008	/* use security namespaces attributes in op */
+
 #endif /* __LIBFROG_ATTR_H__ */
diff --git a/scrub/Makefile b/scrub/Makefile
index 53e8cb02a..1e1109048 100644
--- a/scrub/Makefile
+++ b/scrub/Makefile
@@ -100,10 +100,6 @@ ifeq ($(HAVE_MALLINFO2),yes)
 LCFLAGS += -DHAVE_MALLINFO2
 endif
 
-ifeq ($(HAVE_LIBATTR),yes)
-LCFLAGS += -DHAVE_LIBATTR
-endif
-
 ifeq ($(HAVE_LIBICU),yes)
 CFILES += unicrash.c
 LCFLAGS += -DHAVE_LIBICU $(LIBICU_CFLAGS)
diff --git a/scrub/phase5.c b/scrub/phase5.c
index 27fa29be6..2e495643f 100644
--- a/scrub/phase5.c
+++ b/scrub/phase5.c
@@ -20,6 +20,7 @@
 #include "libfrog/scrub.h"
 #include "libfrog/bitmap.h"
 #include "libfrog/bulkstat.h"
+#include "libfrog/attr.h"
 #include "xfs_scrub.h"
 #include "common.h"
 #include "inodes.h"
@@ -164,7 +165,6 @@ out_unicrash:
 	return ret;
 }
 
-#ifdef HAVE_LIBATTR
 /* Routines to scan all of an inode's xattrs for name problems. */
 struct attrns_decode {
 	int			flags;
@@ -193,8 +193,8 @@ check_xattr_ns_names(
 	struct xfs_attrlist_cursor	cur;
 	char				attrbuf[XFS_XATTR_LIST_MAX];
 	char				keybuf[XATTR_NAME_MAX + 1];
-	struct attrlist			*attrlist = (struct attrlist *)attrbuf;
-	struct attrlist_ent		*ent;
+	struct xfs_attrlist		*attrlist = (struct xfs_attrlist *)attrbuf;
+	struct xfs_attrlist_ent		*ent;
 	struct unicrash			*uc = NULL;
 	int				i;
 	int				error;
@@ -267,9 +267,6 @@ check_xattr_names(
 	}
 	return ret;
 }
-#else
-# define check_xattr_names(c, d, h, b)	(0)
-#endif /* HAVE_LIBATTR */
 
 static int
 render_ino_from_handle(
-- 
2.46.0

Re: [PATCH 1/3] libhandle: Remove libattr dependency

[Christoph Hellwig]

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>

Re: [PATCH 2/3] libfrog: remove libattr dependency

[Christoph Hellwig]

> + * We are redifining here so we don't need to keep libattr as a dependency anymore

> +#define ATTR_ENTRY(buffer, index)		\

Maybe add a XFS_ prefix to distinguish this from the attrlist one
and match the other xfs_ prefixes?

> +	((struct xfs_attrlist_ent *)		\
> +	 &((char *)buffer)[ ((struct xfs_attrlist *)(buffer))->al_offset[index] ])

And maybe this really should be an inline function as well.

> +	struct xfs_attrlist		*attrlist = (struct xfs_attrlist *)attrbuf;

Overly long line

Otherwise this looks good.

Re: [PATCH 3/3] scrub: Remove libattr dependency

[Christoph Hellwig]

>   */
> +
>  #define ATTR_ENTRY(buffer, index)		\

Spurious whitespace change.

>  	((struct xfs_attrlist_ent *)		\
>  	 &((char *)buffer)[ ((struct xfs_attrlist *)(buffer))->al_offset[index] ])
>  
> +/* Attr flags used within xfsprogs, must match the definitions from libattr */
> +#define ATTR_ROOT	0x0002	/* use root namespace attributes in op */
> +#define ATTR_SECURE	0x0008	/* use security namespaces attributes in op */

Why do we need these vs just using XFS_ATTR_ROOT/XFS_ATTR_SECURE from
xfs_da_format.h?

> +	struct xfs_attrlist		*attrlist = (struct xfs_attrlist *)attrbuf;

Overly long line.

Re: [PATCH 2/3] libfrog: remove libattr dependency

[Carlos Maiolino]

On Tue, Aug 27, 2024 at 05:15:08AM GMT, Christoph Hellwig wrote:
> > + * We are redifining here so we don't need to keep libattr as a dependency anymore
> 
> > +#define ATTR_ENTRY(buffer, index)		\
> 
> Maybe add a XFS_ prefix to distinguish this from the attrlist one
> and match the other xfs_ prefixes?

Sounds good, will update it.

> 
> > +	((struct xfs_attrlist_ent *)		\
> > +	 &((char *)buffer)[ ((struct xfs_attrlist *)(buffer))->al_offset[index] ])
> 
> And maybe this really should be an inline function as well.
> 
> > +	struct xfs_attrlist		*attrlist = (struct xfs_attrlist *)attrbuf;
> 
> Overly long line
> 
> Otherwise this looks good.

Thanks for the review, I'll wait for darrick to take a look and will send a V2

Carlos

Re: [PATCH 3/3] scrub: Remove libattr dependency

[Carlos Maiolino]

On Tue, Aug 27, 2024 at 05:16:47AM GMT, Christoph Hellwig wrote:
> >   */
> > +
> >  #define ATTR_ENTRY(buffer, index)		\
> 
> Spurious whitespace change.
> 
> >  	((struct xfs_attrlist_ent *)		\
> >  	 &((char *)buffer)[ ((struct xfs_attrlist *)(buffer))->al_offset[index] ])
> >  
> > +/* Attr flags used within xfsprogs, must match the definitions from libattr */
> > +#define ATTR_ROOT	0x0002	/* use root namespace attributes in op */
> > +#define ATTR_SECURE	0x0008	/* use security namespaces attributes in op */
> 
> Why do we need these vs just using XFS_ATTR_ROOT/XFS_ATTR_SECURE from
> xfs_da_format.h?

Because I didn't see XFS_ATTR_ROOT and XFS_ATTR_SECURE exists :)

I'll take a look on it, and if we don't really need to define ATTR_ROOT/SECURE,
I'll just keep ATTR_ENTRY locally to fsprops.h, we don't need a header file for
just a single definition IMO.

> 
> > +	struct xfs_attrlist		*attrlist = (struct xfs_attrlist *)attrbuf;
> 
> Overly long line.

Ok, Thanks!

Carlos

Re: [PATCH 3/3] scrub: Remove libattr dependency

[Darrick J. Wong]

On Tue, Aug 27, 2024 at 02:27:07PM +0200, Carlos Maiolino wrote:
> On Tue, Aug 27, 2024 at 05:16:47AM GMT, Christoph Hellwig wrote:
> > >   */
> > > +
> > >  #define ATTR_ENTRY(buffer, index)		\
> > 
> > Spurious whitespace change.
> > 
> > >  	((struct xfs_attrlist_ent *)		\
> > >  	 &((char *)buffer)[ ((struct xfs_attrlist *)(buffer))->al_offset[index] ])
> > >  
> > > +/* Attr flags used within xfsprogs, must match the definitions from libattr */
> > > +#define ATTR_ROOT	0x0002	/* use root namespace attributes in op */
> > > +#define ATTR_SECURE	0x0008	/* use security namespaces attributes in op */
> > 
> > Why do we need these vs just using XFS_ATTR_ROOT/XFS_ATTR_SECURE from
> > xfs_da_format.h?
> 
> Because I didn't see XFS_ATTR_ROOT and XFS_ATTR_SECURE exists :)

Well if we're reusing symbols from xfs_fs.h then change these to
XFS_IOC_ATTR_ROOT and XFS_IOC_ATTR_SECURE.

> I'll take a look on it, and if we don't really need to define ATTR_ROOT/SECURE,
> I'll just keep ATTR_ENTRY locally to fsprops.h, we don't need a header file for
> just a single definition IMO.

<shrug> I'd have turned them into namespaced libfrog functions in
libfrog/attr.h, and demacro'd ATTR_ENTRY too:

static inline struct xfs_attrlist_ent *
libfrog_attr_entry(
	struct xfs_attrlist	*list,
	unsigned int		index)
{
	char			*buffer = (char *)list;

	return (struct xfs_attrlist_ent *)&buffer[list->al_offset[index]];
}

static inline int
libfrog_attr_list_by_handle(
	void				*hanp,
	size_t				hlen,
	void				*buf,
	size_t				bufsize,
	int				flags,
	struct xfs_attrlist_cursor	*cursor)
{
	return attr_list_by_handle(hanp, hlen, buf, bufsize, flags,
			(struct attrlist_cursor *)cursor);
}

--D

> 
> > 
> > > +	struct xfs_attrlist		*attrlist = (struct xfs_attrlist *)attrbuf;
> > 
> > Overly long line.
> 
> Ok, Thanks!
> 
> Carlos
> 

Re: [PATCH 1/3] libhandle: Remove libattr dependency

[Darrick J. Wong]

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
> 
> 

Re: [PATCH 1/3] libhandle: Remove libattr dependency

[Darrick J. Wong]

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>
> 
> 

Re: [PATCH 1/3] libhandle: Remove libattr dependency

[Christoph Hellwig]

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..

Re: [PATCH 1/3] libhandle: Remove libattr dependency

[Christoph Hellwig]

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.

Re: [PATCH 3/3] scrub: Remove libattr dependency

[Christoph Hellwig]

On Tue, Aug 27, 2024 at 07:36:15AM -0700, Darrick J. Wong wrote:
> > Because I didn't see XFS_ATTR_ROOT and XFS_ATTR_SECURE exists :)
> 
> Well if we're reusing symbols from xfs_fs.h then change these to
> XFS_IOC_ATTR_ROOT and XFS_IOC_ATTR_SECURE.

Ah, yes.  We've got yet another name for those..

Re: [PATCH 1/3] libhandle: Remove libattr dependency

[Carlos Maiolino]

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