Re: [PATCH v3 0/4] util: Add thread-safe qemu_strerror() function

[Yohei Kojima]

I explain why I did not add "Fixes:" line while it is advised to add
in the previous review. It is because this patch series solves the
issue partially, not completely. There are many more files that
includes `strerror()` call, but changing all of them will result in
the huge patch series that is hard to merge.

In short, fixes line is not added to avoid closing and reopening the
partially fixed issue. The patch series is incomplete because the
complete patch series will be very large.

On 2023/03/31 2:07, Yohei Kojima wrote:
> This patch series adds qemu_strerror() function, which is thread-safe
> version of the libc strerror(). The first patch introduces the
> qemu_strerror() function, and the second patch replaces strerror()
> function in linux-user/* with qemu_strerror() function.
> 
> The difference between this patch series are:
>   1. Add the following patches
>     accel: replace strerror() function to the thread safe qemu_strerror()
>     target/i386: replace strerror() function to the thread safe
>   2. Add `#include "qemu/cutils.h"` line to linux-user/elfload.c
>   3. Fix qemu_strerror() to follow the QEMU coding style
> 
> The following lines are same to the cover letter in the previous
> version.
> 
> Because it involves thread safety, qemu_strerror() should be tested
> carefully. But before adding tests, I want to ask (1) will this patch be
> acceptable to QEMU project after adding tests, (2) where and how
> qemu_strerror() should be tested.
> 
> (1) means that: is my approach too complicated to solve potential
> thread-unsafe implementation of strerror()? Although strerror() is not
> guaranteed to be thread-safe, glibc implements thread-safe strerror().
> We have to consider the balance between maintenance costs and potential
> risks.
> 
> (2) means that: is tests/unit/test-cutils.c a good place for tests?
> Because the behavior of qemu_strerror() is changed by the feature test
> macros, the tests should be run with different test macros, hopefully
> in different OSs.
> 
> Note that strerror_r() function called by qemu_strerror() has
> different return types between architectures because of the historical
> reason. qemu_strerror() handles both the newer POSIX strerror() and the
> older POSIX strerror().
> 
> All tests except for skipped ones are passed in my environment (x86_64
> linux).
> 
> Yohei Kojima (4):
>   util: Add thread-safe qemu_strerror() function
>   linux-user: replace strerror() function to the thread safe
>     qemu_strerror()
>   accel: replace strerror() function to the thread safe qemu_strerror()
>   target/i386: replace strerror() function to the thread safe
>     qemu_strerror()
> 
>  accel/kvm/kvm-all.c               | 32 +++++++++++---------
>  accel/tcg/cputlb.c                |  3 +-
>  accel/tcg/perf.c                  |  7 +++--
>  include/qemu/cutils.h             | 20 +++++++++++++
>  linux-user/elfload.c              |  6 ++--
>  linux-user/main.c                 |  5 ++--
>  linux-user/syscall.c              |  2 +-
>  target/i386/kvm/kvm.c             | 49 ++++++++++++++++---------------
>  target/i386/kvm/xen-emu.c         |  7 +++--
>  target/i386/nvmm/nvmm-accel-ops.c |  2 +-
>  target/i386/sev.c                 |  5 ++--
>  target/i386/whpx/whpx-accel-ops.c |  2 +-
>  util/cutils.c                     | 49 +++++++++++++++++++++++++++++++
>  13 files changed, 136 insertions(+), 53 deletions(-)
>