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

[Nikita Pettik <korablev@tarantool.org>]

On 07 May 23:47, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
> 
> >>> 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
> 
> One last thing - could you please align '\' to the end of the lines?
> By 80 symbols, using tabs. Recently we had a discussion with Cyrill G,

My editor doesn't provide an ability to align by custom offset using
tabs, so I aligned \ by 74 symbols. Feel free to force push fix if
it still looks wrong.

> and he mentioned such not aligned things are hard to read. Physically.
> Eyes literally are getting tired faster.
> 
> He also wants us to align struct members everywhere, but that is a
> different topic to discuss.