From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 0BC4146970E for ; Wed, 15 Jan 2020 16:35:45 +0300 (MSK) Date: Wed, 15 Jan 2020 16:35:49 +0300 From: Alexander Turenko Message-ID: <20200115133548.5gcntnlmvao2eisl@tkn_work_nb> References: <20200110142749.11577-1-gorcunov@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200110142749.11577-1-gorcunov@gmail.com> Subject: Re: [Tarantool-patches] [PATCH] fiber: set diag error on madvise, mprotect fails List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tml My understanding of error handling is so: * Utility functions, simple wrappers and so just set a diag. * A function that able to return an error to a user do it (set or pass through a diag). * A background / delayed function logs an occured errors. So I think that fiber_madvise() and fiber_mprotect() should set a diag: this is correct. Usages of those functions should do one of the following actions: * return -1 (the diag remains set) * ignore the error, but log it: call diag_log() and continue * ignore w/o even logging, but this should be commented in the code As I see, fiber_madvise() log an occured error before the patch, now the error is ignored. I think that doing something like the following in fiber_stack_recycle() and fiber_stack_watermark_create() would be good: | if (fiber_madvise(<...>) != 0) | diag_log(); fiber_mprotect() error is handled in fiber_stack_create() (correctly after the patch), but ignored in fiber_stack_destroy(). Let's log it in the latter function. This is how I understand a right way to handle exceptional situations: don't sure it is right or very consistent with other code. So, please, double-check my proposal. Aside of that I googled a bit whether we can wrap madvise() / mprotect() using LD_PRELOAD for unit testing and it seems it is doable: https://tbrindus.ca/correct-ld-preload-hooking-libc/ I would not bother you with testing of errors in madvise() / mprotect() right now, but it is good topic to experiment around it in a future. See several minor comments below. WBR, Alexander Turenko. > Same time mprotect usage is more sensitive -- we > setup guard pages with its help and when failing > it is definitely an error which we should not only > mention in logger but provide a caller diag error. > Here is an example from memcached Nit: I would say tarantool/memcached just to distinguish from the original memcached software. > issue https://github.com/tarantool/tarantool/issues/4722 > branch gorcunov/gh-4722-mprotect-diag-error > -#define fiber_madvise(addr, len, advice) \ > -({ \ > - int err = madvise((addr), (len), (advice)); \ > - if (err) \ > - say_syserror("madvise"); \ > - err; \ > -}) > - > -#define fiber_mprotect(addr, len, prot) \ > -({ \ > - int err = mprotect((addr), (len), (prot)); \ > - if (err) \ > - say_syserror("mprotect"); \ > - err; \ > -}) > +static inline int > +fiber_madvise(void *addr, size_t len, int advice) > +{ > + if (madvise(addr, len, advice)) { Nit: We usually check against zero explicitly. > + diag_set(SystemError, "fiber madvise %p:%zu:%d failed", > + addr, len, advice); > + return -1; > + } > + return 0; > +} > + > +static inline int > +fiber_mprotect(void *addr, size_t len, int prot) > +{ > + if (mprotect(addr, len, prot)) { Nit: Same here. > + diag_set(SystemError, "fiber mprotect %p:%zu:%d failed", > + addr, len, prot); > + return -1; > + } > + return 0; > +}