Re: [PATCH v3 02/13] tcg: Fix error reporting on mprotect() failure in tcg_region_init()

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 23/9/25 11:09, Markus Armbruster wrote:
> tcg_region_init() calls one of qemu_mprotect_rwx(),
> qemu_mprotect_rw(), and mprotect(), then reports failure with
> error_setg_errno(&error_fatal, errno, ...).
> 
> The use of &error_fatal is undesirable.  qapi/error.h advises:
> 
>   * Please don't error_setg(&error_fatal, ...), use error_report() and
>   * exit(), because that's more obvious.
> 
> The use of errno is wrong.  qemu_mprotect_rwx() and qemu_mprotect_rw()
> wrap around qemu_mprotect__osdep().  qemu_mprotect__osdep() calls
> mprotect() on POSIX, VirtualProtect() on Windows, and reports failure
> with error_report().  VirtualProtect() doesn't set errno.  mprotect()
> does, but error_report() may clobber it.
> 
> Fix tcg_region_init() to report errors only when it calls mprotect(),
> and rely on qemu_mprotect_rwx()'s and qemu_mprotect_rw()'s error
> reporting otherwise.  Use error_report(), not error_setg().
> 
> Fixes: 22c6a9938f75 (tcg: Merge buffer protection and guard page protection)
> Fixes: 6bc144237a85 (tcg: Use Error with alloc_code_gen_buffer)
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   tcg/region.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/tcg/region.c b/tcg/region.c
> index 7ea0b37a84..2181267e48 100644
> --- a/tcg/region.c
> +++ b/tcg/region.c
> @@ -832,13 +832,16 @@ void tcg_region_init(size_t tb_size, int splitwx, unsigned max_threads)
>               } else {
>   #ifdef CONFIG_POSIX
>                   rc = mprotect(start, end - start, need_prot);
> +                if (rc) {
> +                    error_report("mprotect of jit buffer: %s",
> +                                 strerror(errno));

I'm not keen on handling errors differently in the same function.

qemu_mprotect_rwx() and qemu_mprotect_rw() already print the error.

Why not add qemu_mprotect() as a simple qemu_mprotect__osdep() alias,
then call it here, also covering the non-POSIX case?
(Question for Richard, after looking at commits 22c6a9938f7 and more
  importantly 97a83753c9 -- wondering about WoA).

> +                }
>   #else
>                   g_assert_not_reached();
>   #endif
>               }
>               if (rc) {
> -                error_setg_errno(&error_fatal, errno,
> -                                 "mprotect of jit buffer");
> +                exit(1);
>               }>           }
>           if (have_prot != 0) {