[PATCH v2 0/4] xfsprogs: coverity fixes

[PATCH v2 0/4] xfsprogs: coverity fixes

[Bill O'Donnell]

Select fixes made following coverity scan results on 04/23/2024.
CID: 1596597, 1596598, 1596599, 1596600, 159660

Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
---
v2: initialize automatic struct ifake to 0 at declaration instead of
    setting individual member if_levels to 0.
---

[PATCH v2 1/4] mkfs.xfs: avoid potential overflowing expression in xfs_mkfs.c

[Bill O'Donnell]

Cast max_tx_bytes to uint64_t to avoid overflowing expression in
calc_concurrency_logblocks().

Coverity-id: 1596603

Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
---
 mkfs/xfs_mkfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index f4a9bf20..2f801dd4 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3678,7 +3678,7 @@ calc_concurrency_logblocks(
 	 * without blocking for space.  Increase the figure by 50% so that
 	 * background threads can also run.
 	 */
-	log_bytes = max_tx_bytes * 3 * cli->log_concurrency / 2;
+	log_bytes = (uint64_t)max_tx_bytes * 3 * cli->log_concurrency / 2;
 	new_logblocks = min(XFS_MAX_LOG_BYTES >> cfg->blocklog,
 				log_bytes >> cfg->blocklog);
 
-- 
2.45.2

[PATCH v2 2/4] xfs_db: fix unitialized automatic struct ifake to 0.

[Bill O'Donnell]

Ensure automatic struct ifake is properly initialized.

Coverity-id: 1596600, 1596597

Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
---
 db/bmap_inflate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/db/bmap_inflate.c b/db/bmap_inflate.c
index 33b0c954..219f9bbf 100644
--- a/db/bmap_inflate.c
+++ b/db/bmap_inflate.c
@@ -340,7 +340,7 @@ build_new_datafork(
 	const struct xfs_bmbt_irec	*irec,
 	xfs_extnum_t			nextents)
 {
-	struct xbtree_ifakeroot		ifake;
+	struct xbtree_ifakeroot		ifake = {};
 	struct xfs_btree_cur		*bmap_cur;
 	int				error;
 
@@ -394,7 +394,7 @@ estimate_size(
 		.leaf_slack		= 1,
 		.node_slack		= 1,
 	};
-	struct xbtree_ifakeroot		ifake;
+	struct xbtree_ifakeroot		ifake = {};
 	struct xfs_btree_cur		*bmap_cur;
 	int				error;
 
-- 
2.45.2

[PATCH v2 3/4] xfs_fsr: correct type in fsrprintf() call

[Bill O'Donnell]

Use %ld instead of %d for howlong variable.

Coverity-id: 1596598

Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
---
 fsr/xfs_fsr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
index fdd37756..d204e3a4 100644
--- a/fsr/xfs_fsr.c
+++ b/fsr/xfs_fsr.c
@@ -426,7 +426,7 @@ fsrallfs(char *mtab, time_t howlong, char *leftofffile)
 	fsdesc_t *fsp;
 	struct stat sb, sb2;
 
-	fsrprintf("xfs_fsr -m %s -t %d -f %s ...\n", mtab, howlong, leftofffile);
+	fsrprintf("xfs_fsr -m %s -t %ld -f %s ...\n", mtab, howlong, leftofffile);
 
 	endtime = starttime + howlong;
 	fs = fsbase;
-- 
2.45.2

[PATCH v2 4/4] xfs_repair: correct type of variable global_msgs.interval to time_t

[Bill O'Donnell]

Use time_t instead of int for interval field.

Coverity-id: 1596599

Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
---
 repair/progress.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/repair/progress.c b/repair/progress.c
index 2ce36cef..15455a99 100644
--- a/repair/progress.c
+++ b/repair/progress.c
@@ -91,7 +91,7 @@ typedef struct msg_block_s {
 	uint64_t	*done;
 	uint64_t	*total;
 	int		count;
-	int		interval;
+	time_t		interval;
 } msg_block_t;
 static msg_block_t 	global_msgs;
 
-- 
2.45.2

Re: [PATCH v2 1/4] mkfs.xfs: avoid potential overflowing expression in xfs_mkfs.c

[Darrick J. Wong]

On Thu, Jun 13, 2024 at 04:09:15PM -0500, Bill O'Donnell wrote:
> Cast max_tx_bytes to uint64_t to avoid overflowing expression in
> calc_concurrency_logblocks().
> 
> Coverity-id: 1596603
> 
> Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

(didn't hch rvb this earlier?)

--D

> ---
>  mkfs/xfs_mkfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index f4a9bf20..2f801dd4 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3678,7 +3678,7 @@ calc_concurrency_logblocks(
>  	 * without blocking for space.  Increase the figure by 50% so that
>  	 * background threads can also run.
>  	 */
> -	log_bytes = max_tx_bytes * 3 * cli->log_concurrency / 2;
> +	log_bytes = (uint64_t)max_tx_bytes * 3 * cli->log_concurrency / 2;
>  	new_logblocks = min(XFS_MAX_LOG_BYTES >> cfg->blocklog,
>  				log_bytes >> cfg->blocklog);
>  
> -- 
> 2.45.2
> 
> 

Re: [PATCH v2 2/4] xfs_db: fix unitialized automatic struct ifake to 0.

[Darrick J. Wong]

On Thu, Jun 13, 2024 at 04:09:16PM -0500, Bill O'Donnell wrote:
> Ensure automatic struct ifake is properly initialized.
> 
> Coverity-id: 1596600, 1596597
> 
> Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>

Looks good now,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  db/bmap_inflate.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/db/bmap_inflate.c b/db/bmap_inflate.c
> index 33b0c954..219f9bbf 100644
> --- a/db/bmap_inflate.c
> +++ b/db/bmap_inflate.c
> @@ -340,7 +340,7 @@ build_new_datafork(
>  	const struct xfs_bmbt_irec	*irec,
>  	xfs_extnum_t			nextents)
>  {
> -	struct xbtree_ifakeroot		ifake;
> +	struct xbtree_ifakeroot		ifake = {};
>  	struct xfs_btree_cur		*bmap_cur;
>  	int				error;
>  
> @@ -394,7 +394,7 @@ estimate_size(
>  		.leaf_slack		= 1,
>  		.node_slack		= 1,
>  	};
> -	struct xbtree_ifakeroot		ifake;
> +	struct xbtree_ifakeroot		ifake = {};
>  	struct xfs_btree_cur		*bmap_cur;
>  	int				error;
>  
> -- 
> 2.45.2
> 
> 

Re: [PATCH v2 3/4] xfs_fsr: correct type in fsrprintf() call

[Darrick J. Wong]

On Thu, Jun 13, 2024 at 04:09:17PM -0500, Bill O'Donnell wrote:
> Use %ld instead of %d for howlong variable.
> 
> Coverity-id: 1596598
> 
> Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
> ---
>  fsr/xfs_fsr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> index fdd37756..d204e3a4 100644
> --- a/fsr/xfs_fsr.c
> +++ b/fsr/xfs_fsr.c
> @@ -426,7 +426,7 @@ fsrallfs(char *mtab, time_t howlong, char *leftofffile)
>  	fsdesc_t *fsp;
>  	struct stat sb, sb2;
>  
> -	fsrprintf("xfs_fsr -m %s -t %d -f %s ...\n", mtab, howlong, leftofffile);
> +	fsrprintf("xfs_fsr -m %s -t %ld -f %s ...\n", mtab, howlong, leftofffile);

The exact definition of time_t varies by platform and architecture.
I'd paste that is, but in libc it's a twisty mess of indirection that
eventually ends at 'signed long int' or 'long long int'.

Either way, some linter is likely to balk at this, so you might as well
cast howlong to (long long) and use %lld here.

--D

>  
>  	endtime = starttime + howlong;
>  	fs = fsbase;
> -- 
> 2.45.2
> 
> 

Re: [PATCH v2 4/4] xfs_repair: correct type of variable global_msgs.interval to time_t

[Darrick J. Wong]

On Thu, Jun 13, 2024 at 04:09:18PM -0500, Bill O'Donnell wrote:
> Use time_t instead of int for interval field.
> 
> Coverity-id: 1596599
> 
> Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>

Hopefully the intervals never get that big, but yes, we could use a time
interval type instead of the ever popular int.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  repair/progress.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/repair/progress.c b/repair/progress.c
> index 2ce36cef..15455a99 100644
> --- a/repair/progress.c
> +++ b/repair/progress.c
> @@ -91,7 +91,7 @@ typedef struct msg_block_s {
>  	uint64_t	*done;
>  	uint64_t	*total;
>  	int		count;
> -	int		interval;
> +	time_t		interval;
>  } msg_block_t;
>  static msg_block_t 	global_msgs;
>  
> -- 
> 2.45.2
> 
> 

Re: [PATCH v2 3/4] xfs_fsr: correct type in fsrprintf() call

[Bill O'Donnell]

On Thu, Jun 13, 2024 at 08:54:08PM -0700, Darrick J. Wong wrote:
> On Thu, Jun 13, 2024 at 04:09:17PM -0500, Bill O'Donnell wrote:
> > Use %ld instead of %d for howlong variable.
> > 
> > Coverity-id: 1596598
> > 
> > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
> > ---
> >  fsr/xfs_fsr.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> > index fdd37756..d204e3a4 100644
> > --- a/fsr/xfs_fsr.c
> > +++ b/fsr/xfs_fsr.c
> > @@ -426,7 +426,7 @@ fsrallfs(char *mtab, time_t howlong, char *leftofffile)
> >  	fsdesc_t *fsp;
> >  	struct stat sb, sb2;
> >  
> > -	fsrprintf("xfs_fsr -m %s -t %d -f %s ...\n", mtab, howlong, leftofffile);
> > +	fsrprintf("xfs_fsr -m %s -t %ld -f %s ...\n", mtab, howlong, leftofffile);
> 
> The exact definition of time_t varies by platform and architecture.
> I'd paste that is, but in libc it's a twisty mess of indirection that
> eventually ends at 'signed long int' or 'long long int'.
> 
> Either way, some linter is likely to balk at this, so you might as well
> cast howlong to (long long) and use %lld here.

Good idea. I'll send a v3.
Thanks-
Bill


> 
> --D
> 
> >  
> >  	endtime = starttime + howlong;
> >  	fs = fsbase;
> > -- 
> > 2.45.2
> > 
> > 
>