Re: [Tarantool-patches] [PATCH] fiber: set diag error on madvise, mprotect fails

[Alexander Turenko <alexander.turenko@tarantool.org>]

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;
> +}