Re: [PATCH] xfsprogs: Fix mismatched return type of filesize()

Re: [PATCH] xfsprogs: Fix mismatched return type of filesize()

[Theodore Ts'o]

On Fri, Feb 21, 2025 at 07:57:57PM +0100, Pavel Reichl wrote:
> The function filesize() was declared with a return type of 'long' but
> defined with 'off_t'. This mismatch caused build issues due to type
> incompatibility.
> 
> This commit updates the declaration to match the definition, ensuring
> consistency and preventing potential compilation errors.
> 
> Fixes: 73fb78e5ee8 ("mkfs: support copying in large or sparse files")

I had run into this issue when building xfsprogs on i386, and had
investigated the compilation failure before finding this commit in
origin/for-next.  But in my fix, I also found that there was a missing
long -> off_t conversion in setup_proto():

diff --git a/mkfs/proto.c b/mkfs/proto.c
index 7f56a3d8..52ef64ff 100644
--- a/mkfs/proto.c
+++ b/mkfs/proto.c
@@ -61,7 +61,7 @@ setup_proto(
 	char		*buf = NULL;
 	static char	dflt[] = "d--755 0 0 $";
 	int		fd;
-	long		size;
+	off_t		size;
 
 	if (!fname)
 		return dflt;

... since setup_proto() also calls filesize():

	if ((fd = open(fname, O_RDONLY)) < 0 || (size = filesize(fd)) < 0) {

How important is it fix this up?  I can send a formal patch if that
would be helpful, but commit a5466cee9874 is certainly enough to fix
the build failure so maybe it's enough.

Cheers,

					- Ted

Re: [PATCH] xfsprogs: Fix mismatched return type of filesize()

[Carlos Maiolino]

On Tue, Apr 01, 2025 at 08:22:33PM -0400, Theodore Ts'o wrote:
> On Fri, Feb 21, 2025 at 07:57:57PM +0100, Pavel Reichl wrote:
> > The function filesize() was declared with a return type of 'long' but
> > defined with 'off_t'. This mismatch caused build issues due to type
> > incompatibility.
> >
> > This commit updates the declaration to match the definition, ensuring
> > consistency and preventing potential compilation errors.
> >
> > Fixes: 73fb78e5ee8 ("mkfs: support copying in large or sparse files")
> 
> I had run into this issue when building xfsprogs on i386, and had
> investigated the compilation failure before finding this commit in
> origin/for-next.  But in my fix, I also found that there was a missing
> long -> off_t conversion in setup_proto():
> 
> diff --git a/mkfs/proto.c b/mkfs/proto.c
> index 7f56a3d8..52ef64ff 100644
> --- a/mkfs/proto.c
> +++ b/mkfs/proto.c
> @@ -61,7 +61,7 @@ setup_proto(
>  	char		*buf = NULL;
>  	static char	dflt[] = "d--755 0 0 $";
>  	int		fd;
> -	long		size;
> +	off_t		size;
> 
>  	if (!fname)
>  		return dflt;
> 
> ... since setup_proto() also calls filesize():
> 
> 	if ((fd = open(fname, O_RDONLY)) < 0 || (size = filesize(fd)) < 0) {
> 
> How important is it fix this up?  I can send a formal patch if that
> would be helpful, but commit a5466cee9874 is certainly enough to fix
> the build failure so maybe it's enough.

Every fix is welcome, this is something that can clearly be tripped over in the
future.
 
> 
> Cheers,
> 
> 					- Ted

Re: [PATCH] xfsprogs: Fix mismatched return type of filesize()

[Darrick J. Wong]

On Tue, Apr 01, 2025 at 08:22:33PM -0400, Theodore Ts'o wrote:
> On Fri, Feb 21, 2025 at 07:57:57PM +0100, Pavel Reichl wrote:
> > The function filesize() was declared with a return type of 'long' but
> > defined with 'off_t'. This mismatch caused build issues due to type
> > incompatibility.
> > 
> > This commit updates the declaration to match the definition, ensuring
> > consistency and preventing potential compilation errors.
> > 
> > Fixes: 73fb78e5ee8 ("mkfs: support copying in large or sparse files")
> 
> I had run into this issue when building xfsprogs on i386, and had
> investigated the compilation failure before finding this commit in
> origin/for-next.  But in my fix, I also found that there was a missing
> long -> off_t conversion in setup_proto():
> 
> diff --git a/mkfs/proto.c b/mkfs/proto.c
> index 7f56a3d8..52ef64ff 100644
> --- a/mkfs/proto.c
> +++ b/mkfs/proto.c
> @@ -61,7 +61,7 @@ setup_proto(
>  	char		*buf = NULL;
>  	static char	dflt[] = "d--755 0 0 $";
>  	int		fd;
> -	long		size;
> +	off_t		size;
>  
>  	if (!fname)
>  		return dflt;
> 
> ... since setup_proto() also calls filesize():
> 
> 	if ((fd = open(fname, O_RDONLY)) < 0 || (size = filesize(fd)) < 0) {
> 
> How important is it fix this up?  I can send a formal patch if that
> would be helpful, but commit a5466cee9874 is certainly enough to fix
> the build failure so maybe it's enough.

Yes, this is important -- off_t can be larger than long, and that can
result in incorrect truncations.  I hope that nobody will ever pass mkfs
a 5GB protofile on 32-bit, but at least the C type usage could be
correct.

--D

> 
> Cheers,
> 
> 					- Ted
> 

[PATCH] xfsprogs: Fix mismatched return type of filesize()

[Pavel Reichl]

The function filesize() was declared with a return type of 'long' but
defined with 'off_t'. This mismatch caused build issues due to type
incompatibility.

This commit updates the declaration to match the definition, ensuring
consistency and preventing potential compilation errors.

Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
 mkfs/proto.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mkfs/proto.c b/mkfs/proto.c
index 6dd3a200..981f5b11 100644
--- a/mkfs/proto.c
+++ b/mkfs/proto.c
@@ -20,7 +20,7 @@ static struct xfs_trans * getres(struct xfs_mount *mp, uint blocks);
 static void rsvfile(xfs_mount_t *mp, xfs_inode_t *ip, long long len);
 static int newregfile(char **pp, char **fname);
 static void rtinit(xfs_mount_t *mp);
-static long filesize(int fd);
+static off_t filesize(int fd);
 static int slashes_are_spaces;
 
 /*
-- 
2.48.1

Re: [PATCH] xfsprogs: Fix mismatched return type of filesize()

[Carlos Maiolino]

On Mon, Feb 17, 2025 at 04:50:43PM +0100, Pavel Reichl wrote:
> The function filesize() was declared with a return type of 'long' but
> defined with 'off_t'. This mismatch caused build issues due to type
> incompatibility.
> 
> This commit updates the declaration to match the definition, ensuring
> consistency and preventing potential compilation errors.
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>

Looks good, perhaps add:

Fixes: 73fb78e5ee8 ("mkfs: support copying in large or sparse files")

?

Otherwise,
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> ---
>  mkfs/proto.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mkfs/proto.c b/mkfs/proto.c
> index 6dd3a200..981f5b11 100644
> --- a/mkfs/proto.c
> +++ b/mkfs/proto.c
> @@ -20,7 +20,7 @@ static struct xfs_trans * getres(struct xfs_mount *mp, uint blocks);
>  static void rsvfile(xfs_mount_t *mp, xfs_inode_t *ip, long long len);
>  static int newregfile(char **pp, char **fname);
>  static void rtinit(xfs_mount_t *mp);
> -static long filesize(int fd);
> +static off_t filesize(int fd);
>  static int slashes_are_spaces;
> 
>  /*
> --
> 2.48.1
> 
> 

Re: [PATCH] xfsprogs: Fix mismatched return type of filesize()

[Darrick J. Wong]

On Tue, Feb 18, 2025 at 09:18:46AM +0100, Carlos Maiolino wrote:
> On Mon, Feb 17, 2025 at 04:50:43PM +0100, Pavel Reichl wrote:
> > The function filesize() was declared with a return type of 'long' but
> > defined with 'off_t'. This mismatch caused build issues due to type
> > incompatibility.
> > 
> > This commit updates the declaration to match the definition, ensuring
> > consistency and preventing potential compilation errors.
> > 
> > Signed-off-by: Pavel Reichl <preichl@redhat.com>
> 
> Looks good, perhaps add:
> 
> Fixes: 73fb78e5ee8 ("mkfs: support copying in large or sparse files")

Yes please!  Sorry about the discrepancy.

With that fixed,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ?
> 
> Otherwise,
> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> 
> > ---
> >  mkfs/proto.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/mkfs/proto.c b/mkfs/proto.c
> > index 6dd3a200..981f5b11 100644
> > --- a/mkfs/proto.c
> > +++ b/mkfs/proto.c
> > @@ -20,7 +20,7 @@ static struct xfs_trans * getres(struct xfs_mount *mp, uint blocks);
> >  static void rsvfile(xfs_mount_t *mp, xfs_inode_t *ip, long long len);
> >  static int newregfile(char **pp, char **fname);
> >  static void rtinit(xfs_mount_t *mp);
> > -static long filesize(int fd);
> > +static off_t filesize(int fd);
> >  static int slashes_are_spaces;
> > 
> >  /*
> > --
> > 2.48.1
> > 
> > 
>