Re: [PATCH 02/15] mdadm/Grow: fix coverity issue CHECKED_RETURN

[Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>]

On Mon, 15 Jul 2024 15:35:51 +0800
Xiao Ni <xni@redhat.com> wrote:

> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  Grow.c | 43 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 36 insertions(+), 7 deletions(-)
> 
> diff --git a/Grow.c b/Grow.c
> index b135930d05b8..7ae967bda067 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -3261,7 +3261,12 @@ static int reshape_array(char *container, int fd, char
> *devname, /* This is a spare that wants to
>  					 * be part of the array.
>  					 */
> -					add_disk(fd, st, info2, d);
> +					if (add_disk(fd, st, info2, d) < 0) {
> +						pr_err("Can not add disk
> %s\n",
> +								d->sys_name);
> +						free(info2);
> +						goto release;
> +					}
>  				}
>  			}
>  			sysfs_free(info2);
> @@ -4413,7 +4418,10 @@ static void validate(int afd, int bfd, unsigned long
> long offset) */
>  	if (afd < 0)
>  		return;
> -	lseek64(bfd, offset - 4096, 0);
> +	if (lseek64(bfd, offset - 4096, 0) < 0) {
> +		pr_err("lseek64 fails %d:%s\n", errno, strerror(errno));

You are using same error message in many places, shouldn't we propose something
like:

__off64_t lseek64_log_err (int __fd, __off64_t __offset, int __whence)
{
    __off64_t ret = lseek64(fd, __offset, __whence);
    if (ret < 0)
         pr_err("lseek64 fails %d:%s\n", errno, strerror(errno));

    return ret;
    
}

lseek64 errors are unusual, they are exceptional, and I'm fine with logging
same error message but I would prefer to avoid repeating same message in code.
In case of debug, developer can do some backtracking, starting from this
function rather than hunt for the particular error message you used multiple
times.

> +		return;


> +	}
>  	if (read(bfd, &bsb2, 512) != 512)
>  		fail("cannot read bsb");
>  	if (bsb2.sb_csum != bsb_csum((char*)&bsb2,
> @@ -4444,12 +4452,19 @@ static void validate(int afd, int bfd, unsigned long
> long offset) }
>  		}
>  
> -		lseek64(bfd, offset, 0);
> +		if (lseek64(bfd, offset, 0) < 0) {
> +			pr_err("lseek64 fails %d:%s\n", errno,
> strerror(errno));

> +			goto out;
> +		}
>  		if ((unsigned long long)read(bfd, bbuf, len) != len) {
>  			//printf("len %llu\n", len);
>  			fail("read first backup failed");
>  		}
> -		lseek64(afd, __le64_to_cpu(bsb2.arraystart)*512, 0);
> +
> +		if (lseek64(afd, __le64_to_cpu(bsb2.arraystart)*512, 0) < 0)
> {
> +			pr_err("lseek64 fails %d:%s\n", errno,
> strerror(errno));
> +			goto out;
> +		}
>  		if ((unsigned long long)read(afd, abuf, len) != len)
>  			fail("read first from array failed");
>  		if (memcmp(bbuf, abuf, len) != 0)
> @@ -4466,15 +4481,25 @@ static void validate(int afd, int bfd, unsigned long
> long offset) bbuf = xmalloc(abuflen);
>  		}
>  
> -		lseek64(bfd, offset+__le64_to_cpu(bsb2.devstart2)*512, 0);
> +		if (lseek64(bfd, offset+__le64_to_cpu(bsb2.devstart2)*512,
> 0) < 0) {
> +			pr_err("lseek64 fails %d:%s\n", errno,
> strerror(errno));
> +			goto out;
> +		}
>  		if ((unsigned long long)read(bfd, bbuf, len) != len)
>  			fail("read second backup failed");
> -		lseek64(afd, __le64_to_cpu(bsb2.arraystart2)*512, 0);
> +		if (lseek64(afd, __le64_to_cpu(bsb2.arraystart2)*512, 0) <
> 0) {
> +			pr_err("lseek64 fails %d:%s\n", errno,
> strerror(errno));
> +			goto out;
> +		}
>  		if ((unsigned long long)read(afd, abuf, len) != len)
>  			fail("read second from array failed");
>  		if (memcmp(bbuf, abuf, len) != 0)
>  			fail("data2 compare failed");
>  	}
> +out:
> +	free(abuf);
> +	free(bbuf);
> +	return;
>  }
>  
>  int child_monitor(int afd, struct mdinfo *sra, struct reshape *reshape,
> @@ -5033,7 +5058,11 @@ int Grow_continue_command(char *devname, int fd,
> struct context *c) goto Grow_continue_command_exit;
>  		}
>  		content = &array;
> -		sysfs_init(content, fd, NULL);
> +		if (sysfs_init(content, fd, NULL) < 0) {
> +			pr_err("sysfs_init fails\n");

Better error message is:
pr_err("failed to initialize sysfs.\n");
or
pr_err("unable to initialize sysfs for %s\n",st->devnm);

It is more user friendly.

It is already used multiple times so perhaps we can consider similar approach
to proposed in for lseek, we can move move printing error to sysfs_init().

What do you think?

Thanks,
Mariusz