| Message ID | TYZPR06MB5418A6BDB94FB0D97ABA31299D8E9@TYZPR06MB5418.apcprd06.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | util: Add thread-safe qemu_strerror() function | expand |
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>
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> >
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> >>
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> >>> > >
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;
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(+)