| Message ID | 20201026164756.30556-4-mdoucha@suse.cz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Unify error handling in LTP library | expand |
Hi Martin > - Properly format caller file:line locations > - Pedantically check for invalid syscall return values > > Signed-off-by: Martin Doucha<mdoucha@suse.cz> > --- > lib/tst_safe_timerfd.c | 32 +++++++++++++++++++++++--------- > 1 file changed, 23 insertions(+), 9 deletions(-) > > diff --git a/lib/tst_safe_timerfd.c b/lib/tst_safe_timerfd.c > index ffe7b2ef7..4c36a309c 100644 > --- a/lib/tst_safe_timerfd.c > +++ b/lib/tst_safe_timerfd.c > @@ -17,9 +17,14 @@ int safe_timerfd_create(const char *file, const int lineno, > int fd; > > fd = timerfd_create(clockid, flags); > - if (fd< 0) { > - tst_brk(TTYPE | TERRNO, "%s:%d timerfd_create(%s) failed", > - file, lineno, tst_clock_name(clockid)); > + > + if (fd == -1) { > + tst_brk_(file, lineno, TTYPE | TERRNO, > + "timerfd_create(%s) failed", tst_clock_name(clockid)); > + } else if (fd< 0) { > + tst_brk_(file, lineno, TBROK | TERRNO, > + "Invalid timerfd_create(%s) return value %d", > + tst_clock_name(clockid), fd); > } > > return fd; > @@ -31,9 +36,14 @@ int safe_timerfd_gettime(const char *file, const int lineno, > int rval; > > rval = timerfd_gettime(fd, curr_value); > - if (rval != 0) { > - tst_brk(TTYPE | TERRNO, "%s:%d timerfd_gettime() failed", > - file, lineno); > + > + if (rval == -1) { > + tst_brk_(file, lineno, TTYPE | TERRNO, > + "timerfd_gettime() failed"); > + } > + if (rval) { > + tst_brk_(file, lineno, TBROK | TERRNO, > + "Invalid timerfd_gettime() return value %d", rval); Here also should use else if. > } > > return rval; > @@ -47,9 +57,13 @@ int safe_timerfd_settime(const char *file, const int lineno, > int rval; > > rval = timerfd_settime(fd, flags, new_value, old_value); > - if (rval != 0) { > - tst_brk(TTYPE | TERRNO, "%s:%d timerfd_settime() failed", > - file, lineno); > + > + if (rval == -1) { > + tst_brk_(file, lineno, TTYPE | TERRNO, > + "timerfd_settime() failed"); > + } else if (rval) { > + tst_brk_(file, lineno, TBROK | TERRNO, > + "Invalid timerfd_settime() return value %d", rval); > } > > return rval;
Hi Martin, Xu, > > diff --git a/lib/tst_safe_timerfd.c b/lib/tst_safe_timerfd.c > > index ffe7b2ef7..4c36a309c 100644 > > --- a/lib/tst_safe_timerfd.c > > +++ b/lib/tst_safe_timerfd.c > > @@ -17,9 +17,14 @@ int safe_timerfd_create(const char *file, const int lineno, > > int fd; > > fd = timerfd_create(clockid, flags); > > - if (fd< 0) { > > - tst_brk(TTYPE | TERRNO, "%s:%d timerfd_create(%s) failed", > > - file, lineno, tst_clock_name(clockid)); > > + > > + if (fd == -1) { > > + tst_brk_(file, lineno, TTYPE | TERRNO, > > + "timerfd_create(%s) failed", tst_clock_name(clockid)); > > + } else if (fd< 0) { nit: fd< 0 => fd < 0 > > + tst_brk_(file, lineno, TBROK | TERRNO, > > + "Invalid timerfd_create(%s) return value %d", > > + tst_clock_name(clockid), fd); > > } > > return fd; > > @@ -31,9 +36,14 @@ int safe_timerfd_gettime(const char *file, const int lineno, > > int rval; > > rval = timerfd_gettime(fd, curr_value); > > - if (rval != 0) { > > - tst_brk(TTYPE | TERRNO, "%s:%d timerfd_gettime() failed", > > - file, lineno); > > + > > + if (rval == -1) { > > + tst_brk_(file, lineno, TTYPE | TERRNO, > > + "timerfd_gettime() failed"); > > + } > > + if (rval) { > > + tst_brk_(file, lineno, TBROK | TERRNO, > > + "Invalid timerfd_gettime() return value %d", rval); > Here also should use else if. +1. If I'm not wrong we need to count that safe functions does not quit the test in cleanup function (tst_brk__() and tst_brk_entered). Kind regards, Petr
Hi Petr, Martin > Hi Martin, Xu, > >>> diff --git a/lib/tst_safe_timerfd.c b/lib/tst_safe_timerfd.c >>> index ffe7b2ef7..4c36a309c 100644 >>> --- a/lib/tst_safe_timerfd.c >>> +++ b/lib/tst_safe_timerfd.c >>> @@ -17,9 +17,14 @@ int safe_timerfd_create(const char *file, const int lineno, >>> int fd; > >>> fd = timerfd_create(clockid, flags); >>> - if (fd< 0) { >>> - tst_brk(TTYPE | TERRNO, "%s:%d timerfd_create(%s) failed", >>> - file, lineno, tst_clock_name(clockid)); >>> + >>> + if (fd == -1) { >>> + tst_brk_(file, lineno, TTYPE | TERRNO, >>> + "timerfd_create(%s) failed", tst_clock_name(clockid)); >>> + } else if (fd< 0) { > nit: fd< 0 => fd< 0 Sorry, It is my email format problem, on patchwork[1], the format is right. [1]https://patchwork.ozlabs.org/project/ltp/patch/20201026164756.30556-4-mdoucha@suse.cz/ >>> + tst_brk_(file, lineno, TBROK | TERRNO, >>> + "Invalid timerfd_create(%s) return value %d", >>> + tst_clock_name(clockid), fd); > >>> } > >>> return fd; >>> @@ -31,9 +36,14 @@ int safe_timerfd_gettime(const char *file, const int lineno, >>> int rval; > >>> rval = timerfd_gettime(fd, curr_value); >>> - if (rval != 0) { >>> - tst_brk(TTYPE | TERRNO, "%s:%d timerfd_gettime() failed", >>> - file, lineno); >>> + >>> + if (rval == -1) { >>> + tst_brk_(file, lineno, TTYPE | TERRNO, >>> + "timerfd_gettime() failed"); >>> + } >>> + if (rval) { >>> + tst_brk_(file, lineno, TBROK | TERRNO, >>> + "Invalid timerfd_gettime() return value %d", rval); >> Here also should use else if. > +1. > If I'm not wrong we need to count that safe functions does not quit the test in > cleanup function (tst_brk__() and tst_brk_entered). Yes, because we want to do more it in cleanup, so we use "tst_brk_handler = tst_cvres" to print something instead of " tst_brk_handler = tst_vbrk_;". We also have documented it in doc/test-writing-guidelines.txt. Best Regards Yang Xu > > Kind regards, > Petr > > > . >
Hi Martin, Xu, > > Hi Martin, Xu, > > > > diff --git a/lib/tst_safe_timerfd.c b/lib/tst_safe_timerfd.c > > > > index ffe7b2ef7..4c36a309c 100644 > > > > --- a/lib/tst_safe_timerfd.c > > > > +++ b/lib/tst_safe_timerfd.c > > > > @@ -17,9 +17,14 @@ int safe_timerfd_create(const char *file, const int lineno, > > > > int fd; > > > > fd = timerfd_create(clockid, flags); > > > > - if (fd< 0) { > > > > - tst_brk(TTYPE | TERRNO, "%s:%d timerfd_create(%s) failed", > > > > - file, lineno, tst_clock_name(clockid)); > > > > + > > > > + if (fd == -1) { > > > > + tst_brk_(file, lineno, TTYPE | TERRNO, > > > > + "timerfd_create(%s) failed", tst_clock_name(clockid)); > > > > + } else if (fd< 0) { > > nit: fd< 0 => fd< 0 > Sorry, It is my email format problem, on patchwork[1], the format is right. Martin, sorry for not checking what you send in the patch. > [1]https://patchwork.ozlabs.org/project/ltp/patch/20201026164756.30556-4-mdoucha@suse.cz/ Kind regards, Petr
diff --git a/lib/tst_safe_timerfd.c b/lib/tst_safe_timerfd.c index ffe7b2ef7..4c36a309c 100644 --- a/lib/tst_safe_timerfd.c +++ b/lib/tst_safe_timerfd.c @@ -17,9 +17,14 @@ int safe_timerfd_create(const char *file, const int lineno, int fd; fd = timerfd_create(clockid, flags); - if (fd < 0) { - tst_brk(TTYPE | TERRNO, "%s:%d timerfd_create(%s) failed", - file, lineno, tst_clock_name(clockid)); + + if (fd == -1) { + tst_brk_(file, lineno, TTYPE | TERRNO, + "timerfd_create(%s) failed", tst_clock_name(clockid)); + } else if (fd < 0) { + tst_brk_(file, lineno, TBROK | TERRNO, + "Invalid timerfd_create(%s) return value %d", + tst_clock_name(clockid), fd); } return fd; @@ -31,9 +36,14 @@ int safe_timerfd_gettime(const char *file, const int lineno, int rval; rval = timerfd_gettime(fd, curr_value); - if (rval != 0) { - tst_brk(TTYPE | TERRNO, "%s:%d timerfd_gettime() failed", - file, lineno); + + if (rval == -1) { + tst_brk_(file, lineno, TTYPE | TERRNO, + "timerfd_gettime() failed"); + } + if (rval) { + tst_brk_(file, lineno, TBROK | TERRNO, + "Invalid timerfd_gettime() return value %d", rval); } return rval; @@ -47,9 +57,13 @@ int safe_timerfd_settime(const char *file, const int lineno, int rval; rval = timerfd_settime(fd, flags, new_value, old_value); - if (rval != 0) { - tst_brk(TTYPE | TERRNO, "%s:%d timerfd_settime() failed", - file, lineno); + + if (rval == -1) { + tst_brk_(file, lineno, TTYPE | TERRNO, + "timerfd_settime() failed"); + } else if (rval) { + tst_brk_(file, lineno, TBROK | TERRNO, + "Invalid timerfd_settime() return value %d", rval); } return rval;
- Properly format caller file:line locations - Pedantically check for invalid syscall return values Signed-off-by: Martin Doucha <mdoucha@suse.cz> --- lib/tst_safe_timerfd.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)