<div dir="ltr"><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_extra">Cyril Hrubis <span dir="ltr"><<a href="mailto:chrubis@suse.cz" target="_blank">chrubis@suse.cz</a>></span> wrote:<br><span class="gmail-m_-8514980772807819230gmail-"></span><br><span class="gmail-m_-8514980772807819230gmail-"></span><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-m_-8514980772807819230gmail-">
> +#define TST_RETRY_FUNC_WITH_EXPONENTIA<wbr>L_BACKOFF(FUNC, ERET, MAX_DELAY) \<br>
<br>
</span>That is quite a long name, I guess that we can shorten it a bit without<br>
loosing too much clarity. Something as TST_RETRY_FN_EXP_BACKOFF()<br>
carries all the information and is a bit shorter.<br></blockquote><div><div style="display:inline" class="gmail_default"><span style="font-family:arial,helvetica,sans-serif">​<br></span></div><div style="display:inline" class="gmail_default"><span style="font-family:arial,helvetica,sans-serif">Yes, and we can also take use this directly for looping more seconds.<br><br></span></div><span style="font-family:arial,helvetica,sans-serif"></span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="gmail-m_-8514980772807819230gmail-"><br>
> +do { int delay = 1;                                          \<br>
<br>
</span>We should at least prefix the delay with tst_ to make sure that it will<br>
not alias with anything that has been passed to the FUNC, e.g. if the<br>
FUNC is defined as foo_func(delay); the delay variable will be aliased<br>
and the function will do something very unexpected.<br></blockquote><div><div class="gmail_default"><span style="font-family:arial,helvetica,sans-serif">​<br></span></div><div class="gmail_default"><span style="font-family:arial,helvetica,sans-serif">You are right, I missed this problem. <br></span></div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="gmail-m_-8514980772807819230gmail-"><br>
> +     for (;;) {                                              \<br>
> +             typeof(FUNC) ret = FUNC;                        \<br>
> +             if (ret == (typeof(FUNC))ERET)                  \<br>
> +                     break;                                  \<br>
<br>
</span>Do we really need the (typeof(FUNC)) cast here?<br></blockquote><div class="gmail_default"><span style="font-family:arial,helvetica,sans-serif">​<br>I set the type cast to ensure more safe in comparing, but it seems no needed.<br></span></div><div class="gmail_default"><span style="font-family:arial,helvetica,sans-serif">Will remove this.<br><br></span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="gmail-m_-8514980772807819230gmail-"><br>
> +             if (delay < MAX_DELAY * 1000000) {              \<br>
> +                     usleep(delay);                          \<br>
> +                     delay *= 2;                             \<br>
<br>
</span>Maybe we can be a bit more verbose and say something as:<br>
<br>
tst_res(TINFO, #FUNC" returned %i, retrying in %ius", ret, delay);<br></blockquote><div><div style="display:inline" class="gmail_default"><span style="font-family:arial,helvetica,sans-serif"><br>Agree​.<br><br></span></div><span style="font-family:arial,helvetica,sans-serif"></span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="gmail-m_-8514980772807819230gmail-"><br>
> +             } else {                                        \<br>
> +                     tst_brk_(__FILE__, __LINE__,            \<br>
> +                     TBROK | TERRNO, #FUNC " failed");       \<br>
<br>
</span>As far as I can tell we can just use the plain tst_brk() in macros since<br>
the __LINE__ will be a constant in the whole macro and will represent<br>
the line where the macro is called.<br>
<br>
Also I'm not sure that adding the TERRNO here is a good idea, I suppose<br>
that there may be a retry functions that are not seting it on a failure.<br>
Maybe we can pass additional tst_res/tst_brk flags to the macro itself.<br>
<span class="gmail-m_-8514980772807819230gmail-"><br></span></blockquote><div><br><div style="font-family:arial,helvetica,sans-serif;display:inline" class="gmail_default">​Hmm, yes! But I don't want to add more flags to the macro since that will<br></div><div style="font-family:arial,helvetica,sans-serif;display:inline" class="gmail_default">make things more complicated for user. So, let's just abandon the TERRNO<br>to see how thing going on.<br></div><div style="font-family:arial,helvetica,sans-serif;display:inline" class="gmail_default">​</div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-m_-8514980772807819230gmail-">
> +             }                                               \<br>
> +     }                                                       \<br>
> +} while(0)<br>
> +<br>
> +/*<br>
> + * Exponential backoff usleep for wating a varible change<br>
> + * @VAR: the variable will be changed in other place<br>
> + * @EXP: an expected value which VAR should be equal to<br>
> + */<br>
> +#define TST_WAIT_ON_VAR(VAR, EXP)    \<br>
> +     TST_WAIT_WITH_EXPONENTIAL_BAC<wbr>KOFF(VAR, EXP, 1)<br>
> +<br>
> +#define TST_WAIT_WITH_EXPONENTIAL_BACK<wbr>OFF(VAR, EXP, MAX_DELAY)       \<br>
> +do { int delay = 1;                                          \<br>
</span>                      ^<br>
                      Trailing whitespace.<br>
<span class="gmail-m_-8514980772807819230gmail-">> +     for (;;) {                                              \<br>
> +             if (VAR == (typeof(VAR)) EXP)                   \<br>
> +                     break;                                  \<br>
> +             if (delay < MAX_DELAY * 1000000) {              \<br>
> +                     usleep(delay);                          \<br>
> +                     delay *= 2;                             \<br>
> +             } else {                                        \<br>
> +                     tst_brk_(__FILE__, __LINE__,            \<br>
> +                     TBROK | TERRNO, #VAR " is not expected");       \<br>
> +             }                                               \<br>
> +     }                                                       \<br>
> +} while(0)<br>
<br>
</span>I would refrain from adding this function unless we have a use-case<br>
already. Do you have a test in mind that could use this?<br></blockquote><div><span style="font-family:arial,helvetica,sans-serif"><br>No, I don't have any test so far, this is just came to my mind</span><br><span style="font-family:arial,helvetica,sans-serif"></span><span style="font-family:arial,helvetica,sans-serif"></span><div style="display:inline" class="gmail_default"><span style="font-family:arial,helvetica,sans-serif">for one situation. We could re-add it if necessary next time.​</span></div><span style="font-family:arial,helvetica,sans-serif"> </span><br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class="gmail-m_-8514980772807819230gmail-HOEnZb"><div class="gmail-m_-8514980772807819230gmail-h5"><br>
>  #endif /* TST_COMMON_H__ */<br>
> --<br>
> 1.9.3<br>
><br>
<br>
--<br>
</div></div><span class="gmail-m_-8514980772807819230gmail-HOEnZb"><font color="#888888">Cyril Hrubis<br>
<a href="mailto:chrubis@suse.cz" target="_blank">chrubis@suse.cz</a><br>
</font></span></blockquote></div><br><br clear="all"><br>-- <br><div class="gmail-m_-8514980772807819230gmail_signature">Li Wang<br><a href="mailto:liwang@redhat.com" target="_blank">liwang@redhat.com</a></div>
</div></div>