diff mbox series

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

Message ID 20240715073604.30307-3-xni@redhat.com (mailing list archive)
State Superseded
Headers show
Series mdadm: fix coverity issues | expand

Checks

Context Check Description
mdraidci/vmtest-md-6_11-PR fail merge-conflict

Commit Message

Xiao Ni July 15, 2024, 7:35 a.m. UTC 
Signed-off-by: Xiao Ni <xni@redhat.com>
---
 Grow.c | 43 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 7 deletions(-)

Comments

Mariusz Tkaczyk July 17, 2024, 9:33 a.m. UTC | #1 
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
Xiao Ni July 18, 2024, 2:29 a.m. UTC | #2 
On Wed, Jul 17, 2024 at 5:33 PM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> 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.

Hi Mariusz

If we use the above way, pr_err only prints the line of code in the
function lseek64_log_err. We can't know where the error is. pr_err
prints the function name and code line number. We put pr_err in the
place where lseek fails, we can know which function and which lseek
fails. It should be easy for debug. Is it right?
>
> > +             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.

Yes, it makes sense.
>
> 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?

It depends on the above answer you'll give.

Best Regards
Xiao
>
> Thanks,
> Mariusz
>
Mariusz Tkaczyk July 18, 2024, 7:19 a.m. UTC | #3 
On Thu, 18 Jul 2024 10:29:50 +0800
Xiao Ni <xni@redhat.com> wrote:

> > 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.  
> 
> Hi Mariusz
> 
> If we use the above way, pr_err only prints the line of code in the
> function lseek64_log_err. We can't know where the error is. pr_err
> prints the function name and code line number. We put pr_err in the
> place where lseek fails, we can know which function and which lseek
> fails. It should be easy for debug. Is it right?
> >  

Oh right, I forgot about this from simple reason - I'm never using this.
Line is printed with make CXFLAGS=DDEBUG. If you will enable DEBUG flag then
mdadm will also spam with debug messages (prr_dbg messages). Anyway, you are
right it is an option.

Enabling DDEBUG in the past many times broke my time sensitive reproductions. I
don't like it.

From the top of my head, there is a rule "do not repeat same code". If we can
avoid repeating same error message by wrapping that to function then you will
satisfy my perfectionist sense. That's all.

I'm not going to force you to follow it - you proved that there is a way to
hunt the line by DDEBUG so I'm satisfied.

Thanks,
Mariusz
diff mbox series

Patch

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));
+		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");
+			ret_val = 1;
+			goto Grow_continue_command_exit;
+		}
 		/* Need to load a superblock.
 		 * FIXME we should really get what we need from
 		 * sysfs