Re: [Tarantool-patches] [PATCH 1/2] errinj: introduce delayed injection

[Nikita Pettik <korablev@tarantool.org>]

On 03 May 21:20, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> > diff --git a/src/errinj.h b/src/errinj.h
> > index 2cb090b68..990f4921d 100644
> > --- a/src/errinj.h
> > +++ b/src/errinj.h
> > @@ -149,6 +149,7 @@ errinj_foreach(errinj_cb cb, void *cb_ctx);
> >  #ifdef NDEBUG
> >  #  define ERROR_INJECT(ID, CODE)
> >  #  define errinj(ID, TYPE) ((struct errinj *) NULL)
> > +#  define ERROR_INJECT_DELAYED(ID, DELAY, CODE)
> >  #else
> >  #  /* Returns the error injection by id */
> >  #  define errinj(ID, TYPE) \
> > @@ -162,6 +163,14 @@ errinj_foreach(errinj_cb cb, void *cb_ctx);
> >  		if (errinj(ID, ERRINJ_BOOL)->bparam) \
> >  			CODE; \
> >  	} while (0)
> > +#  define ERROR_INJECT_DELAYED(ID, DELAY, CODE) \
> > +	do { \
> > +		static int _DELAY##ID = DELAY; \
> > +		if (errinj(ID, ERRINJ_BOOL)->bparam) { \
> > +			if (_DELAY##ID-- == 0) \
> > +				CODE; \
> > +		} \
> > +	} while (0)
> 
> The method is fine unless you will ever want to set the same
> error injection twice without restarting Tarantool. With this
> solution there is no way to reset _DELAY##ID back to the same or
> a different value. It will stay 0 until restart. This a serious
> enough problem to reconsider the approach. There are injections
> used in multiple tests, and we can't count on restart after each
> one.

Yep, this is obvious drawback of chosen implementation.
 
> This is the reason why you won't be able to use this delayed
> injection for VY_STMT_ALLOC in 'uninitialized memory' patchset.
> 
> 'Delayed' easily can be implemented by iparam. For example, look
> at ERRINJ_WAL_FALLOCATE and your own VY_STMT_ALLOC. You set
> iparam to some value, then you decrement it until 0, and inject
> the error at this moment. Works perfectly fine, and without
> introducing any new unrecoverable ERRINJ_ counters. However if you
> want to simplify that, just wrap the iparam-method into a macros.
> Even the same name could be fine - ERROR_INJECT_DELAYED.

Ok, here's diff:

diff --git a/src/errinj.h b/src/errinj.h
index 990f4921d..945abb939 100644
--- a/src/errinj.h
+++ b/src/errinj.h
@@ -163,12 +163,10 @@ errinj_foreach(errinj_cb cb, void *cb_ctx);
                if (errinj(ID, ERRINJ_BOOL)->bparam) \
                        CODE; \
        } while (0)
-#  define ERROR_INJECT_DELAYED(ID, DELAY, CODE) \
+#  define ERROR_INJECT_COUNTDOWN(ID, CODE) \
        do { \
-               static int _DELAY##ID = DELAY; \
-               if (errinj(ID, ERRINJ_BOOL)->bparam) { \
-                       if (_DELAY##ID-- == 0) \
-                               CODE; \
+               if (errinj(ID, ERRINJ_INT)->iparam-- == 0) { \
+                       CODE; \
                } \
        } while (0)
 #endif


> Moreover, it simplifies check that the injection really happened,
> and its progress. You just look at iparam in Lua and see how many
> times it has executed, and whether it is equal to 0 already
> (with bparam you can do the same though, by setting it to false
> explicitly).