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

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

[Yohei Kojima]

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(-)

-- 
2.39.2

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

[Yohei Kojima]

Add qemu_strerror() which follows the POSIX specification for
strerror(). While strerror() is not guaranteed to be thread-safe, this
function is thread-safe.

This function is added to solve the following issue:
https://gitlab.com/qemu-project/qemu/-/issues/416

Signed-off-by: Yohei Kojima <y-koj@outlook.jp>
---
 include/qemu/cutils.h | 20 ++++++++++++++++++
 util/cutils.c         | 49 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 92c436d8c7..0bcae0049a 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -117,6 +117,26 @@ int stristart(const char *str, const char *val, const char **ptr);
  * Returns: length of @s in bytes, or @max_len, whichever is smaller.
  */
 int qemu_strnlen(const char *s, int max_len);
+/**
+ * qemu_strerror:
+ * @errnum: error number
+ *
+ * Return the pointer to a string that describes errnum, like
+ * strerror(). This function is thread-safe because the buffer for
+ * returned string is allocated per thread.
+ *
+ * This function is thread-safe, but not reentrant. In other words,
+ * if a thread is interrupted by a signal in this function, and the
+ * thread calls this function again in the signal handling, then
+ * the result might be corrupted.
+ *
+ * This function has the same behaviour as the POSIX strerror()
+ * function.
+ *
+ * Returns: the pointer to an error description, or an
+ * "Unknown error nnn" message if errnum is invalid.
+ */
+char *qemu_strerror(int errnum);
 /**
  * qemu_strsep:
  * @input: pointer to string to parse
diff --git a/util/cutils.c b/util/cutils.c
index 5887e74414..571edd768b 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -131,6 +131,55 @@ int qemu_strnlen(const char *s, int max_len)
     return i;
 }
 
+/**
+ * It assumes the length of error descriptions are at most 1024.
+ * The concern of write buffer overflow is cared by strerror_r().
+ */
+static __thread char qemu_strerror_buf[1024];
+
+#if (_POSIX_C_SOURCE >= 200112L) && !_GNU_SOURCE
+/**
+ * In POSIX.1-2001 and after, the return type of strerror_r is int, but
+ * glibc overrides the definition of strerror_r to the old strerror_r
+ * if _GNU_SOURCE is defined. This condition handles it.
+ */
+
+char *qemu_strerror(int errnum)
+{
+    int is_error = strerror_r(errnum, qemu_strerror_buf, 1024);
+
+    if (is_error == 0) {
+        return qemu_strerror_buf;
+    }
+
+    /**
+     * handle the error occured in strerror_r
+     *
+     * If is_error is greater than 0, errno might not be updated by
+     * strerror_r. Otherwise, errno is updated.
+     */
+    if (is_error > 0) {
+        errno = is_error;
+    }
+
+    strncpy(qemu_strerror_buf, "Error %d occured\n", errno);
+    return qemu_strerror_buf;
+}
+#else
+/**
+ * In glibc, the return type of strerror_r is char* if _GNU_SOURCE
+ * is defined. In this case, strerror_r returns qemu_strerror_buf iff
+ * some error occured in strerror_r, and otherwise it returns a pointer
+ * to the pre-defined description for errnum.
+ *
+ * This is the same behaviour until POSIX.1-2001.
+ */
+char *qemu_strerror(int errnum)
+{
+    return strerror_r(errnum, qemu_strerror_buf, 1024);
+}
+#endif
+
 char *qemu_strsep(char **input, const char *delim)
 {
     char *result = *input;
-- 
2.39.2

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

[Markus Armbruster]

Yohei Kojima <y-koj@outlook.jp> writes:

> Add qemu_strerror() which follows the POSIX specification for
> strerror(). While strerror() is not guaranteed to be thread-safe, this
> function is thread-safe.

Why not g_strerror()?

> This function is added to solve the following issue:
> https://gitlab.com/qemu-project/qemu/-/issues/416

The issue even asks for it...

> Signed-off-by: Yohei Kojima <y-koj@outlook.jp>

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

[Yohei Kojima]

On 2023/03/31 4:06, Markus Armbruster wrote:
> Yohei Kojima <y-koj@outlook.jp> writes:
> 
>> Add qemu_strerror() which follows the POSIX specification for
>> strerror(). While strerror() is not guaranteed to be thread-safe, this
>> function is thread-safe.
> 
> Why not g_strerror()?
> 

Because g_strerror() uses mutex in its implementation and there is a
risk to occur the deadlock. If one thread enters g_strerror() (soon the
mutex is locked), then another thread calls fork(), and the forked
child process try to call g_strerror(), then deadlock occurs.

>> This function is added to solve the following issue:
>> https://gitlab.com/qemu-project/qemu/-/issues/416
> 
> The issue even asks for it...
> 

Originally yes, but Daniel told the deadlock (or a mutex starvation)
risk for g_strerror() in the later discussion of the issue. Probably I
should have mention that in the commit message or the cover letter.

>> Signed-off-by: Yohei Kojima <y-koj@outlook.jp>
> 

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

[Alex Bennée]

Yohei Kojima <y-koj@outlook.jp> writes:

> On 2023/03/31 4:06, Markus Armbruster wrote:
>> Yohei Kojima <y-koj@outlook.jp> writes:
>> 
>>> Add qemu_strerror() which follows the POSIX specification for
>>> strerror(). While strerror() is not guaranteed to be thread-safe, this
>>> function is thread-safe.
>> 
>> Why not g_strerror()?
>> 
>
> Because g_strerror() uses mutex in its implementation and there is a
> risk to occur the deadlock. If one thread enters g_strerror() (soon the
> mutex is locked), then another thread calls fork(), and the forked
> child process try to call g_strerror(), then deadlock occurs.

I think we should mention this avoids the deadlock in the commit
message. With that:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


>
>>> This function is added to solve the following issue:
>>> https://gitlab.com/qemu-project/qemu/-/issues/416
>> 
>> The issue even asks for it...
>> 
>
> Originally yes, but Daniel told the deadlock (or a mutex starvation)
> risk for g_strerror() in the later discussion of the issue. Probably I
> should have mention that in the commit message or the cover letter.
>
>>> Signed-off-by: Yohei Kojima <y-koj@outlook.jp>
>> 


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

[Yohei Kojima]

On 2023/04/06 17:57, Alex Bennée wrote:
> 
> Yohei Kojima <y-koj@outlook.jp> writes:
> 
>> On 2023/03/31 4:06, Markus Armbruster wrote:
>>> Yohei Kojima <y-koj@outlook.jp> writes:
>>>
>>>> Add qemu_strerror() which follows the POSIX specification for
>>>> strerror(). While strerror() is not guaranteed to be thread-safe, this
>>>> function is thread-safe.
>>>
>>> Why not g_strerror()?
>>>
>>
>> Because g_strerror() uses mutex in its implementation and there is a
>> risk to occur the deadlock. If one thread enters g_strerror() (soon the
>> mutex is locked), then another thread calls fork(), and the forked
>> child process try to call g_strerror(), then deadlock occurs.
> 
> I think we should mention this avoids the deadlock in the commit
> message. With that:
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Thank you for the review. I will address that in the next version.

> 
> 
>>
>>>> This function is added to solve the following issue:
>>>> https://gitlab.com/qemu-project/qemu/-/issues/416
>>>
>>> The issue even asks for it...
>>>
>>
>> Originally yes, but Daniel told the deadlock (or a mutex starvation)
>> risk for g_strerror() in the later discussion of the issue. Probably I
>> should have mention that in the commit message or the cover letter.
>>
>>>> Signed-off-by: Yohei Kojima <y-koj@outlook.jp>
>>>
> 
> 

[PATCH v3 2/4] linux-user: replace strerror() function to the thread safe qemu_strerror()

[Yohei Kojima]

strerror() is not guaranteed to be thread-safe as described in
(https://gitlab.com/qemu-project/qemu/-/issues/416).

This commit changes files under /linux-user that call strerror() to call
the safer qemu_strerror().

Signed-off-by: Yohei Kojima <y-koj@outlook.jp>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
 linux-user/elfload.c | 6 ++++--
 linux-user/main.c    | 5 +++--
 linux-user/syscall.c | 2 +-
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index b96b3e566b..0fde7de735 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -17,6 +17,7 @@
 #include "qemu/guest-random.h"
 #include "qemu/units.h"
 #include "qemu/selfmap.h"
+#include "qemu/cutils.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "target_signal.h"
@@ -2790,7 +2791,8 @@ static void pgb_reserved_va(const char *image_name, abi_ulong guest_loaddr,
         error_report("Unable to reserve 0x%lx bytes of virtual address "
                      "space at %p (%s) for use as guest address space (check your "
                      "virtual memory ulimit setting, min_mmap_addr or reserve less "
-                     "using -R option)", reserved_va + 1, test, strerror(errno));
+                     "using -R option)", reserved_va + 1, test,
+                     qemu_strerror(errno));
         exit(EXIT_FAILURE);
     }
 
@@ -3583,7 +3585,7 @@ int load_elf_binary(struct linux_binprm *bprm, struct image_info *info)
     g_free(scratch);
 
     if (!bprm->p) {
-        fprintf(stderr, "%s: %s\n", bprm->filename, strerror(E2BIG));
+        fprintf(stderr, "%s: %s\n", bprm->filename, qemu_strerror(E2BIG));
         exit(-1);
     }
 
diff --git a/linux-user/main.c b/linux-user/main.c
index fe03293516..953b8ede65 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -746,7 +746,8 @@ int main(int argc, char **argv, char **envp)
     if (execfd == 0) {
         execfd = open(exec_path, O_RDONLY);
         if (execfd < 0) {
-            printf("Error while loading %s: %s\n", exec_path, strerror(errno));
+            printf("Error while loading %s: %s\n", exec_path,
+                   qemu_strerror(errno));
             _exit(EXIT_FAILURE);
         }
     }
@@ -892,7 +893,7 @@ int main(int argc, char **argv, char **envp)
     ret = loader_exec(execfd, exec_path, target_argv, target_environ, regs,
         info, &bprm);
     if (ret != 0) {
-        printf("Error while loading %s: %s\n", exec_path, strerror(-ret));
+        printf("Error while loading %s: %s\n", exec_path, qemu_strerror(-ret));
         _exit(EXIT_FAILURE);
     }
 
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 69f740ff98..f6b4fe8059 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -581,7 +581,7 @@ const char *target_strerror(int err)
         return "Successful exit from sigreturn";
     }
 
-    return strerror(target_to_host_errno(err));
+    return qemu_strerror(target_to_host_errno(err));
 }
 
 static int check_zeroed_user(abi_long addr, size_t ksize, size_t usize)
-- 
2.39.2

[PATCH v3 3/4] accel: replace strerror() function to the thread safe qemu_strerror()

[Yohei Kojima]

strerror() is not guaranteed to be thread-safe as described in
(https://gitlab.com/qemu-project/qemu/-/issues/416).

This commit changes files under /accel that call strerror() to call
the safer qemu_strerror().

Signed-off-by: Yohei Kojima <y-koj@outlook.jp>
---
 accel/kvm/kvm-all.c | 32 ++++++++++++++++++--------------
 accel/tcg/cputlb.c  |  3 ++-
 accel/tcg/perf.c    |  7 ++++---
 3 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index f2a6ea6a68..b3dc7743db 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -22,6 +22,7 @@
 #include "qemu/atomic.h"
 #include "qemu/option.h"
 #include "qemu/config-file.h"
+#include "qemu/cutils.h"
 #include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "hw/pci/msi.h"
@@ -315,7 +316,7 @@ err:
         error_report("%s: KVM_SET_USER_MEMORY_REGION failed, slot=%d,"
                      " start=0x%" PRIx64 ", size=0x%" PRIx64 ": %s",
                      __func__, mem.slot, slot->start_addr,
-                     (uint64_t)mem.memory_size, strerror(errno));
+                     (uint64_t)mem.memory_size, qemu_strerror(errno));
     }
     return ret;
 }
@@ -1366,7 +1367,7 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
             err = kvm_set_user_memory_region(kml, mem, false);
             if (err) {
                 fprintf(stderr, "%s: error unregistering slot: %s\n",
-                        __func__, strerror(-err));
+                        __func__, qemu_strerror(-err));
                 abort();
             }
             start_addr += slot_size;
@@ -1389,7 +1390,7 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
         err = kvm_set_user_memory_region(kml, mem, true);
         if (err) {
             fprintf(stderr, "%s: error registering slot: %s\n", __func__,
-                    strerror(-err));
+                    qemu_strerror(-err));
             abort();
         }
         start_addr += slot_size;
@@ -1613,7 +1614,7 @@ static void kvm_mem_ioeventfd_add(MemoryListener *listener,
                                match_data);
     if (r < 0) {
         fprintf(stderr, "%s: error adding ioeventfd: %s (%d)\n",
-                __func__, strerror(-r), -r);
+                __func__, qemu_strerror(-r), -r);
         abort();
     }
 }
@@ -1649,7 +1650,7 @@ static void kvm_io_ioeventfd_add(MemoryListener *listener,
                               match_data);
     if (r < 0) {
         fprintf(stderr, "%s: error adding ioeventfd: %s (%d)\n",
-                __func__, strerror(-r), -r);
+                __func__, qemu_strerror(-r), -r);
         abort();
     }
 }
@@ -1668,7 +1669,7 @@ static void kvm_io_ioeventfd_del(MemoryListener *listener,
                               match_data);
     if (r < 0) {
         fprintf(stderr, "%s: error deleting ioeventfd: %s (%d)\n",
-                __func__, strerror(-r), -r);
+                __func__, qemu_strerror(-r), -r);
         abort();
     }
 }
@@ -2278,7 +2279,8 @@ static void kvm_irqchip_create(KVMState *s)
     } else if (kvm_check_extension(s, KVM_CAP_S390_IRQCHIP)) {
         ret = kvm_vm_enable_cap(s, KVM_CAP_S390_IRQCHIP, 0);
         if (ret < 0) {
-            fprintf(stderr, "Enable kernel irqchip failed: %s\n", strerror(-ret));
+            fprintf(stderr, "Enable kernel irqchip failed: %s\n",
+                    qemu_strerror(-ret));
             exit(1);
         }
     } else {
@@ -2297,7 +2299,8 @@ static void kvm_irqchip_create(KVMState *s)
         }
     }
     if (ret < 0) {
-        fprintf(stderr, "Create kernel irqchip failed: %s\n", strerror(-ret));
+        fprintf(stderr, "Create kernel irqchip failed: %s\n",
+                qemu_strerror(-ret));
         exit(1);
     }
 
@@ -2446,7 +2449,7 @@ static int kvm_init(MachineState *ms)
 
     if (ret < 0) {
         fprintf(stderr, "ioctl(KVM_CREATE_VM) failed: %d %s\n", -ret,
-                strerror(-ret));
+                qemu_strerror(-ret));
 
 #ifdef TARGET_S390X
         if (ret == -EINVAL) {
@@ -2532,7 +2535,8 @@ static int kvm_init(MachineState *ms)
             ret = kvm_vm_enable_cap(s, KVM_CAP_DIRTY_LOG_RING, 0, ring_bytes);
             if (ret) {
                 error_report("Enabling of KVM dirty ring failed: %s. "
-                             "Suggested minimum value is 1024.", strerror(-ret));
+                             "Suggested minimum value is 1024.",
+                             qemu_strerror(-ret));
                 goto err;
             }
 
@@ -2949,7 +2953,7 @@ int kvm_cpu_exec(CPUState *cpu)
                 break;
             }
             fprintf(stderr, "error: kvm run failed %s\n",
-                    strerror(-run_ret));
+                    qemu_strerror(-run_ret));
 #ifdef TARGET_PPC
             if (run_ret == -EBUSY) {
                 fprintf(stderr,
@@ -3455,7 +3459,7 @@ void kvm_init_cpu_signals(CPUState *cpu)
         r = kvm_set_signal_mask(cpu, &set);
     }
     if (r) {
-        fprintf(stderr, "kvm_set_signal_mask: %s\n", strerror(-r));
+        fprintf(stderr, "kvm_set_signal_mask: %s\n", qemu_strerror(-r));
         exit(1);
     }
 }
@@ -3538,7 +3542,7 @@ int kvm_set_one_reg(CPUState *cs, uint64_t id, void *source)
     reg.addr = (uintptr_t) source;
     r = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
     if (r) {
-        trace_kvm_failed_reg_set(id, strerror(-r));
+        trace_kvm_failed_reg_set(id, qemu_strerror(-r));
     }
     return r;
 }
@@ -3552,7 +3556,7 @@ int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target)
     reg.addr = (uintptr_t) target;
     r = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
     if (r) {
-        trace_kvm_failed_reg_get(id, strerror(-r));
+        trace_kvm_failed_reg_get(id, qemu_strerror(-r));
     }
     return r;
 }
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index e984a98dc4..6cf888cdf1 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -40,6 +40,7 @@
 #include "qemu/plugin-memory.h"
 #endif
 #include "tcg/tcg-ldst.h"
+#include "qemu/cutils.h"
 
 /* DEBUG defines, enable DEBUG_TLB_LOG to log to the CPU_LOG_MMU target */
 /* #define DEBUG_TLB */
@@ -215,7 +216,7 @@ static void tlb_mmu_resize_locked(CPUTLBDesc *desc, CPUTLBDescFast *fast,
      */
     while (fast->table == NULL || desc->fulltlb == NULL) {
         if (new_size == (1 << CPU_TLB_DYN_MIN_BITS)) {
-            error_report("%s: %s", __func__, strerror(errno));
+            error_report("%s: %s", __func__, qemu_strerror(errno));
             abort();
         }
         new_size = MAX(new_size >> 1, 1 << CPU_TLB_DYN_MIN_BITS);
diff --git a/accel/tcg/perf.c b/accel/tcg/perf.c
index 65e35ea3b9..0c7a3a8822 100644
--- a/accel/tcg/perf.c
+++ b/accel/tcg/perf.c
@@ -13,6 +13,7 @@
 #include "exec/exec-all.h"
 #include "qemu/timer.h"
 #include "tcg/tcg.h"
+#include "qemu/cutils.h"
 
 #include "debuginfo.h"
 #include "perf.h"
@@ -54,7 +55,7 @@ void perf_enable_perfmap(void)
     perfmap = safe_fopen_w(map_file);
     if (perfmap == NULL) {
         warn_report("Could not open %s: %s, proceeding without perfmap",
-                    map_file, strerror(errno));
+                    map_file, qemu_strerror(errno));
     }
 }
 
@@ -201,7 +202,7 @@ void perf_enable_jitdump(void)
     jitdump = safe_fopen_w(jitdump_file);
     if (jitdump == NULL) {
         warn_report("Could not open %s: %s, proceeding without jitdump",
-                    jitdump_file, strerror(errno));
+                    jitdump_file, qemu_strerror(errno));
         return;
     }
 
@@ -214,7 +215,7 @@ void perf_enable_jitdump(void)
                        MAP_PRIVATE, fileno(jitdump), 0);
     if (perf_marker == MAP_FAILED) {
         warn_report("Could not map %s: %s, proceeding without jitdump",
-                    jitdump_file, strerror(errno));
+                    jitdump_file, qemu_strerror(errno));
         fclose(jitdump);
         jitdump = NULL;
         return;
-- 
2.39.2

[PATCH v3 4/4] target/i386: replace strerror() function to the thread safe qemu_strerror()

[Yohei Kojima]

strerror() is not guaranteed to be thread-safe as described in
(https://gitlab.com/qemu-project/qemu/-/issues/416).

This commit changes files under /target/i386 that call strerror() to
call the safer qemu_strerror().

Signed-off-by: Yohei Kojima <y-koj@outlook.jp>
---
 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 +-
 5 files changed, 35 insertions(+), 30 deletions(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index de531842f6..b31810f108 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -41,6 +41,7 @@
 #include "qemu/main-loop.h"
 #include "qemu/ratelimit.h"
 #include "qemu/config-file.h"
+#include "qemu/cutils.h"
 #include "qemu/error-report.h"
 #include "qemu/memalign.h"
 #include "hw/i386/x86.h"
@@ -275,7 +276,7 @@ static struct kvm_cpuid2 *try_get_cpuid(KVMState *s, int max)
             return NULL;
         } else {
             fprintf(stderr, "KVM_GET_SUPPORTED_CPUID failed: %s\n",
-                    strerror(-r));
+                    qemu_strerror(-r));
             exit(1);
         }
     }
@@ -519,7 +520,7 @@ uint64_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index)
     ret = kvm_ioctl(s, KVM_GET_MSRS, &msr_data);
     if (ret != 1) {
         error_report("KVM get MSR (index=0x%x) feature failed, %s",
-            index, strerror(-ret));
+            index, qemu_strerror(-ret));
         exit(1);
     }
 
@@ -1055,7 +1056,7 @@ static struct kvm_cpuid2 *try_get_hv_cpuid(CPUState *cs, int max,
             return NULL;
         } else {
             fprintf(stderr, "KVM_GET_SUPPORTED_HV_CPUID failed: %s\n",
-                    strerror(-r));
+                    qemu_strerror(-r));
             exit(1);
         }
     }
@@ -1642,7 +1643,7 @@ static int hyperv_init_vcpu(X86CPU *cpu)
         ret = kvm_vcpu_enable_cap(cs, synic_cap, 0);
         if (ret < 0) {
             error_report("failed to turn on HyperV SynIC in KVM: %s",
-                         strerror(-ret));
+                         qemu_strerror(-ret));
             return ret;
         }
 
@@ -1650,7 +1651,7 @@ static int hyperv_init_vcpu(X86CPU *cpu)
             ret = hyperv_x86_synic_add(cpu);
             if (ret < 0) {
                 error_report("failed to create HyperV SynIC: %s",
-                             strerror(-ret));
+                             qemu_strerror(-ret));
                 return ret;
             }
         }
@@ -1689,7 +1690,7 @@ static int hyperv_init_vcpu(X86CPU *cpu)
         ret = kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENFORCE_CPUID, 0, 1);
         if (ret < 0) {
             error_report("failed to enable KVM_CAP_HYPERV_ENFORCE_CPUID: %s",
-                         strerror(-ret));
+                         qemu_strerror(-ret));
             return ret;
         }
     }
@@ -1918,7 +1919,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
         if (r < 0) {
             fprintf(stderr,
                     "failed to enable KVM_CAP_ENFORCE_PV_FEATURE_CPUID: %s",
-                    strerror(-r));
+                    qemu_strerror(-r));
             abort();
         }
     }
@@ -2156,7 +2157,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
 
         ret = kvm_get_mce_cap_supported(cs->kvm_state, &mcg_cap, &banks);
         if (ret < 0) {
-            fprintf(stderr, "kvm_get_mce_cap_supported: %s", strerror(-ret));
+            fprintf(stderr, "kvm_get_mce_cap_supported: %s",
+                    qemu_strerror(-ret));
             return ret;
         }
 
@@ -2179,7 +2181,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
         env->mcg_cap &= mcg_cap | MCG_CAP_BANKS_MASK;
         ret = kvm_vcpu_ioctl(cs, KVM_X86_SETUP_MCE, &env->mcg_cap);
         if (ret < 0) {
-            fprintf(stderr, "KVM_X86_SETUP_MCE: %s", strerror(-ret));
+            fprintf(stderr, "KVM_X86_SETUP_MCE: %s", qemu_strerror(-ret));
             return ret;
         }
     }
@@ -2354,7 +2356,7 @@ static int kvm_get_supported_feature_msrs(KVMState *s)
     ret = kvm_ioctl(s, KVM_GET_MSR_FEATURE_INDEX_LIST, &msr_list);
     if (ret < 0 && ret != -E2BIG) {
         error_report("Fetch KVM feature MSR list failed: %s",
-            strerror(-ret));
+            qemu_strerror(-ret));
         return ret;
     }
 
@@ -2367,7 +2369,7 @@ static int kvm_get_supported_feature_msrs(KVMState *s)
 
     if (ret < 0) {
         error_report("Fetch KVM feature MSR list failed: %s",
-            strerror(-ret));
+            qemu_strerror(-ret));
         g_free(kvm_feature_msrs);
         kvm_feature_msrs = NULL;
         return ret;
@@ -2595,7 +2597,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
         ret = kvm_vm_enable_cap(s, KVM_CAP_EXCEPTION_PAYLOAD, 0, true);
         if (ret < 0) {
             error_report("kvm: Failed to enable exception payload cap: %s",
-                         strerror(-ret));
+                         qemu_strerror(-ret));
             return ret;
         }
     }
@@ -2605,7 +2607,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
         ret = kvm_vm_enable_cap(s, KVM_CAP_X86_TRIPLE_FAULT_EVENT, 0, true);
         if (ret < 0) {
             error_report("kvm: Failed to enable triple fault event cap: %s",
-                         strerror(-ret));
+                         qemu_strerror(-ret));
             return ret;
         }
     }
@@ -2707,7 +2709,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
                                 disable_exits);
         if (ret < 0) {
             error_report("kvm: guest stopping CPU not supported: %s",
-                         strerror(-ret));
+                         qemu_strerror(-ret));
         }
     }
 
@@ -2724,7 +2726,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
                                     KVM_BUS_LOCK_DETECTION_EXIT);
             if (ret < 0) {
                 error_report("kvm: Failed to enable bus lock detection cap: %s",
-                             strerror(-ret));
+                             qemu_strerror(-ret));
                 return ret;
             }
             ratelimit_init(&bus_lock_ratelimit_ctrl);
@@ -2743,7 +2745,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
                                     notify_window_flags);
             if (ret < 0) {
                 error_report("kvm: Failed to enable notify vmexit cap: %s",
-                             strerror(-ret));
+                             qemu_strerror(-ret));
                 return ret;
             }
     }
@@ -2754,7 +2756,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
                                 KVM_MSR_EXIT_REASON_FILTER);
         if (ret) {
             error_report("Could not enable user space MSRs: %s",
-                         strerror(-ret));
+                         qemu_strerror(-ret));
             exit(1);
         }
 
@@ -2762,7 +2764,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
                            kvm_rdmsr_core_thread_count, NULL);
         if (!r) {
             error_report("Could not install MSR_CORE_THREAD_COUNT handler: %s",
-                         strerror(-ret));
+                         qemu_strerror(-ret));
             exit(1);
         }
     }
@@ -4889,7 +4891,7 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
             ret = kvm_vcpu_ioctl(cpu, KVM_NMI);
             if (ret < 0) {
                 fprintf(stderr, "KVM: injection failed, NMI lost (%s)\n",
-                        strerror(-ret));
+                        qemu_strerror(-ret));
             }
         }
         if (cpu->interrupt_request & CPU_INTERRUPT_SMI) {
@@ -4900,7 +4902,7 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
             ret = kvm_vcpu_ioctl(cpu, KVM_SMI);
             if (ret < 0) {
                 fprintf(stderr, "KVM: injection failed, SMI lost (%s)\n",
-                        strerror(-ret));
+                        qemu_strerror(-ret));
             }
         }
     }
@@ -4941,7 +4943,7 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
                 if (ret < 0) {
                     fprintf(stderr,
                             "KVM: injection failed, interrupt lost (%s)\n",
-                            strerror(-ret));
+                            qemu_strerror(-ret));
                 }
             }
         }
@@ -5414,7 +5416,8 @@ static bool __kvm_enable_sgx_provisioning(KVMState *s)
 
     ret = kvm_vm_enable_cap(s, KVM_CAP_SGX_ATTRIBUTE, 0, fd);
     if (ret) {
-        error_report("Could not enable SGX PROVISIONKEY: %s", strerror(-ret));
+        error_report("Could not enable SGX PROVISIONKEY: %s",
+                     qemu_strerror(-ret));
         exit(1);
     }
     close(fd);
@@ -5580,7 +5583,7 @@ int kvm_arch_irqchip_create(KVMState *s)
         ret = kvm_vm_enable_cap(s, KVM_CAP_SPLIT_IRQCHIP, 0, 24);
         if (ret) {
             error_report("Could not enable split irqchip mode: %s",
-                         strerror(-ret));
+                         qemu_strerror(-ret));
             exit(1);
         } else {
             DPRINTF("Enabled KVM_CAP_SPLIT_IRQCHIP\n");
diff --git a/target/i386/kvm/xen-emu.c b/target/i386/kvm/xen-emu.c
index d7c7eb8d9c..f7837ff95a 100644
--- a/target/i386/kvm/xen-emu.c
+++ b/target/i386/kvm/xen-emu.c
@@ -12,6 +12,7 @@
 #include "qemu/osdep.h"
 #include "qemu/log.h"
 #include "qemu/main-loop.h"
+#include "qemu/cutils.h"
 #include "qemu/error-report.h"
 #include "hw/xen/xen.h"
 #include "sysemu/kvm_int.h"
@@ -135,7 +136,7 @@ int kvm_xen_init(KVMState *s, uint32_t hypercall_msr)
     ret = kvm_vm_ioctl(s, KVM_XEN_HVM_CONFIG, &cfg);
     if (ret < 0) {
         error_report("kvm: Failed to enable Xen HVM support: %s",
-                     strerror(-ret));
+                     qemu_strerror(-ret));
         return ret;
     }
 
@@ -209,7 +210,7 @@ int kvm_xen_init_vcpu(CPUState *cs)
         err = kvm_vcpu_ioctl(cs, KVM_XEN_VCPU_SET_ATTR, &va);
         if (err) {
             error_report("kvm: Failed to set Xen vCPU ID attribute: %s",
-                         strerror(-err));
+                         qemu_strerror(-err));
             return err;
         }
     }
@@ -964,7 +965,7 @@ static uint64_t kvm_get_current_ns(void)
 
     ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
     if (ret < 0) {
-        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
+        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", qemu_strerror(ret));
                 abort();
     }
 
diff --git a/target/i386/nvmm/nvmm-accel-ops.c b/target/i386/nvmm/nvmm-accel-ops.c
index 6c46101ac1..97d9daacea 100644
--- a/target/i386/nvmm/nvmm-accel-ops.c
+++ b/target/i386/nvmm/nvmm-accel-ops.c
@@ -32,7 +32,7 @@ static void *qemu_nvmm_cpu_thread_fn(void *arg)
 
     r = nvmm_init_vcpu(cpu);
     if (r < 0) {
-        fprintf(stderr, "nvmm_init_vcpu failed: %s\n", strerror(-r));
+        fprintf(stderr, "nvmm_init_vcpu failed: %s\n", qemu_strerror(-r));
         exit(1);
     }
 
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 859e06f6ad..9f19fc5469 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -38,6 +38,7 @@
 #include "exec/confidential-guest-support.h"
 #include "hw/i386/pc.h"
 #include "exec/address-spaces.h"
+#include "qemu/cutils.h"
 
 #define TYPE_SEV_GUEST "sev-guest"
 OBJECT_DECLARE_SIMPLE_TYPE(SevGuestState, SEV_GUEST)
@@ -247,7 +248,7 @@ sev_ram_block_added(RAMBlockNotifier *n, void *host, size_t size,
     r = kvm_vm_ioctl(kvm_state, KVM_MEMORY_ENCRYPT_REG_REGION, &range);
     if (r) {
         error_report("%s: failed to register region (%p+%#zx) error '%s'",
-                     __func__, host, max_size, strerror(errno));
+                     __func__, host, max_size, qemu_strerror(errno));
         exit(1);
     }
 }
@@ -948,7 +949,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
     sev->sev_fd = open(devname, O_RDWR);
     if (sev->sev_fd < 0) {
         error_setg(errp, "%s: Failed to open %s '%s'", __func__,
-                   devname, strerror(errno));
+                   devname, qemu_strerror(errno));
         g_free(devname);
         goto err;
     }
diff --git a/target/i386/whpx/whpx-accel-ops.c b/target/i386/whpx/whpx-accel-ops.c
index e8dc4b3a47..bf16c8e643 100644
--- a/target/i386/whpx/whpx-accel-ops.c
+++ b/target/i386/whpx/whpx-accel-ops.c
@@ -32,7 +32,7 @@ static void *whpx_cpu_thread_fn(void *arg)
 
     r = whpx_init_vcpu(cpu);
     if (r < 0) {
-        fprintf(stderr, "whpx_init_vcpu failed: %s\n", strerror(-r));
+        fprintf(stderr, "whpx_init_vcpu failed: %s\n", qemu_strerror(-r));
         exit(1);
     }
 
-- 
2.39.2

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(-)
>