From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [94.100.177.107]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 79608469710 for ; Fri, 8 May 2020 01:41:18 +0300 (MSK) Date: Thu, 7 May 2020 22:41:17 +0000 From: Nikita Pettik Message-ID: <20200507224117.GD13970@tarantool.org> References: <1fb821b72a258f638109ea5e2e9a6bfd6106b9da.1588273848.git.korablev@tarantool.org> <20200507135001.GC11724@tarantool.org> <5601c484-849d-5425-cdf9-bb5080160c03@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <5601c484-849d-5425-cdf9-bb5080160c03@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 1/2] errinj: introduce delayed injection List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.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.