[2/2] c: Use $LTP_TIMEOUT_MUL also in retry functions
diff mbox series

Message ID 20190718083943.7687-2-pvorel@suse.cz
State Superseded
Delegated to: Petr Vorel
Headers show
Series
  • [1/2] shell: Use $LTP_TIMEOUT_MUL also in retry functions
Related show

Commit Message

Petr Vorel July 18, 2019, 8:39 a.m. UTC
in TST_RETRY_FN_EXP_BACKOFF() and thus in TST_RETRY_FUNC() in new C API.
This sync C API with shell API (added in previous commit).

+ mention LTP_TIMEOUT_MUL use in wiki.
+ use SPDX, add copyright

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 doc/test-writing-guidelines.txt |  2 ++
 include/tst_common.h            | 26 +++++++++++---------------
 2 files changed, 13 insertions(+), 15 deletions(-)

Comments

Li Wang July 19, 2019, 5:02 a.m. UTC | #1
On Thu, Jul 18, 2019 at 4:40 PM Petr Vorel <pvorel@suse.cz> wrote:
>...
>  #ifndef TST_COMMON_H__
> @@ -51,15 +40,22 @@
>
>  #define TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, MAX_DELAY)        \
>  ({     int tst_delay_ = 1;                                             \
> +       float m = 1;    \
> +       char *mul = getenv("LTP_TIMEOUT_MUL");  \

We also need a prefix/suffix in the variable definition to make sure
that it will
not alias with anything that has been passed to the FUNC, just like what we do
for the tst_delay_.

e.g. if the FUNC is defined as foo_func(m); the m variable will be aliased and
the function will do something very unexpected.

> +       if (mul) {      \
> +               m = atof(mul); \
> +               if (m < 1) \
> +                       tst_brk(TBROK, "Invalid timeout multiplier '%s'", mul); \

If we reverse some code order in tst_set_timeout() function, then here
we have no need to check if m < 1 again, since the LTP_TIMEOUT_MUL
valid check will be finished in setup() early phase.
(This comment is just FYI, and I also think it's OK to check twice.)

--------------------------------------
void tst_set_timeout(int timeout)
{
        float m = 1;
        char *mul = getenv("LTP_TIMEOUT_MUL");

        if (mul) {
                m = atof(mul);
                if (m < 1)
                        tst_brk(TBROK, "Invalid timeout multiplier '%s'", mul);
        }

        if (timeout == -1) {
                tst_res(TINFO, "Timeout per run is disabled");
                return;
        }

        results->timeout = timeout * m + 0.5;

        tst_res(TINFO, "Timeout per run is %uh %02um %02us",
                results->timeout/3600, (results->timeout%3600)/60,
                results->timeout % 60);

        if (getpid() == lib_pid)
                alarm(results->timeout);
        else
                heartbeat();
}


--
Regards,
Li Wang
Petr Vorel July 19, 2019, 9:15 a.m. UTC | #2
Hi Li,

thanks for your review!

> On Thu, Jul 18, 2019 at 4:40 PM Petr Vorel <pvorel@suse.cz> wrote:
> >...
> >  #ifndef TST_COMMON_H__
> > @@ -51,15 +40,22 @@

> >  #define TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, MAX_DELAY)        \
> >  ({     int tst_delay_ = 1;                                             \
> > +       float m = 1;    \
> > +       char *mul = getenv("LTP_TIMEOUT_MUL");  \

> We also need a prefix/suffix in the variable definition to make sure
> that it will
> not alias with anything that has been passed to the FUNC, just like what we do
> for the tst_delay_.

> e.g. if the FUNC is defined as foo_func(m); the m variable will be aliased and
> the function will do something very unexpected.
Good point, I'll fix it in v3.

> > +       if (mul) {      \
> > +               m = atof(mul); \
> > +               if (m < 1) \
> > +                       tst_brk(TBROK, "Invalid timeout multiplier '%s'", mul); \

> If we reverse some code order in tst_set_timeout() function, then here
> we have no need to check if m < 1 again, since the LTP_TIMEOUT_MUL
> valid check will be finished in setup() early phase.
> (This comment is just FYI, and I also think it's OK to check twice.)
Good point. I'd probably check twice in case the logic changes one day.

> --------------------------------------
> void tst_set_timeout(int timeout)
> {
>         float m = 1;
>         char *mul = getenv("LTP_TIMEOUT_MUL");

>         if (mul) {
>                 m = atof(mul);
>                 if (m < 1)
>                         tst_brk(TBROK, "Invalid timeout multiplier '%s'", mul);
>         }

>         if (timeout == -1) {
>                 tst_res(TINFO, "Timeout per run is disabled");
>                 return;
>         }

>         results->timeout = timeout * m + 0.5;

>         tst_res(TINFO, "Timeout per run is %uh %02um %02us",
>                 results->timeout/3600, (results->timeout%3600)/60,
>                 results->timeout % 60);

>         if (getpid() == lib_pid)
>                 alarm(results->timeout);
>         else
>                 heartbeat();
> }


Kind regards,
Petr

Patch
diff mbox series

diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index 782e14f32..3548d6043 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -2064,6 +2064,8 @@  Sometimes LTP test needs retrying a function for many times to get success.
 This achievement makes that possible via keeping it retrying if the return
 value of the function is NOT as we expected. After exceeding a limited time,
 test will break from the retries immediately.
+NOTE: both 'TST_RETRY_FUNC()' and 'TST_RETRY_FN_EXP_BACKOFF()' use 'LTP_TIMEOUT_MUL'
+environment variable.
 
 [source,c]
 -------------------------------------------------------------------------------
diff --git a/include/tst_common.h b/include/tst_common.h
index c21505450..454a166e8 100644
--- a/include/tst_common.h
+++ b/include/tst_common.h
@@ -1,21 +1,10 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
+ * Copyright (c) Linux Test Project, 2017-2019
  * Copyright (c) 2016 Cyril Hrubis <chrubis@suse.cz>
  * Copyright (c) 2013 Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
  * Copyright (c) 2010 Ngie Cooper <yaneurabeya@gmail.com>
  * Copyright (c) 2008 Mike Frysinger <vapier@gmail.com>
- *
- * This program is free software: you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation, either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program. If not, see <http://www.gnu.org/licenses/>.
  */
 
 #ifndef TST_COMMON_H__
@@ -51,15 +40,22 @@ 
 
 #define TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, MAX_DELAY)	\
 ({	int tst_delay_ = 1;						\
+	float m = 1;	\
+	char *mul = getenv("LTP_TIMEOUT_MUL");	\
+	if (mul) {	\
+		m = atof(mul); \
+		if (m < 1) \
+			tst_brk(TBROK, "Invalid timeout multiplier '%s'", mul); \
+	}	\
 	for (;;) {							\
 		typeof(FUNC) tst_ret_ = FUNC;				\
 		if (tst_ret_ == ERET)					\
 			break;						\
-		if (tst_delay_ < MAX_DELAY * 1000000) {			\
+		if (tst_delay_ < MAX_DELAY * m * 1000000) {			\
 			usleep(tst_delay_);				\
 			tst_delay_ *= 2;				\
 		} else {						\
-			tst_brk(TBROK, #FUNC" timed out");		\
+			tst_brk(TBROK, #FUNC" timed out! If you are running on slow machine, try exporting LTP_TIMEOUT_MUL > 1"); \
 		}							\
 	}								\
 	ERET;								\