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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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,
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.