On Wed, Jul 17, 2024 at 5:33 PM Mariusz Tkaczyk
<mariusz.tkaczyk@xxxxxxxxxxxxxxx> wrote:
>
> On Mon, 15 Jul 2024 15:35:51 +0800
> Xiao Ni <xni@xxxxxxxxxx> wrote:
>
> > Signed-off-by: Xiao Ni <xni@xxxxxxxxxx>
> > ---
> > 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
>
