[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC v4 7/7] qemu_thread_create: propagate theerr
From: |
Fei Li |
Subject: |
Re: [Qemu-devel] [PATCH RFC v4 7/7] qemu_thread_create: propagate theerror to callers to handle |
Date: |
Sun, 30 Sep 2018 01:38:54 -0600 |
On 09/29/2018 11:04 AM, Fam Zheng wrote:
> On Wed, Sep 26, 2018 at 7:13 PM Fei Li <address@hidden> wrote:
>>
>>
>> On 09/26/2018 06:36 PM, Fam Zheng wrote:
>>> On Wed, 09/26 18:02, Fei Li wrote:
>>>> diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
>>>> index 289af4fab5..8b044e2798 100644
>>>> --- a/util/qemu-thread-posix.c
>>>> +++ b/util/qemu-thread-posix.c
>>>> @@ -15,6 +15,7 @@
>>>> #include "qemu/atomic.h"
>>>> #include "qemu/notify.h"
>>>> #include "qemu-thread-common.h"
>>>> +#include "qapi/error.h"
>>>>
>>>> static bool name_threads;
>>>>
>>>> @@ -504,9 +505,9 @@ static void *qemu_thread_start(void *args)
>>>> return start_routine(arg);
>>>> }
>>>>
>>>> -void qemu_thread_create(QemuThread *thread, const char *name,
>>>> - void *(*start_routine)(void*),
>>>> - void *arg, int mode)
>>>> +bool qemu_thread_create(QemuThread *thread, const char *name,
>>>> + void *(*start_routine)(void *),
>>>> + void *arg, int mode, Error **errp)
>>>> {
>>>> sigset_t set, oldset;
>>>> int err;
>>>> @@ -515,7 +516,9 @@ void qemu_thread_create(QemuThread *thread, const char
>>>> *name,
>>>>
>>>> err = pthread_attr_init(&attr);
>>>> if (err) {
>>>> - error_exit(err, __func__);
>>>> + error_setg(errp, "pthread_attr_init failed: %s", strerror(err));
>>> This can use error_setg_errno.
ok.
>>>
>>>> + errno = err;
>>> Is errno used anywhere in this series? The windows implementation doesn't
>>> set
>>> it.
>> Yes, this is used for the return value in qemu_signal_init() for patch
>> 1/7, I use
>> "return -1" for the further judgement in qemu_init_main_loop() in
>> previous versions
>> but in this version I keep "return -errno" and add the "errno = err"
>> here, just as I
>> explained in [v3 1/7]. ;)
> This makes an inconsistent function contract: on error, does errno get
> set? For Linux implementation it is yes, but for Windows it is no.
> Which I think is wrong. Am I missing anything?
>
> Fam
You are right. Sorry that I only explained why adding the errno = err
for linux,
somehow overlooked the latter part "The windows implementaion ...".
Now three options are in my mind:
- add the "errno = GetLastError()" for windows too
- change "return -errno;" to "return -1;" in qemu_signal_init() in this
patch but not in 1/7 patch
- change "return -errno;" to "return -1" in qemu_signal_init() in 1/7 patch
Which one do you think is better?
Have a nice day, thanks for the advice :)
Fei
>
>>>> + return false;
>>>> }
>>>>
>>>> if (mode == QEMU_THREAD_DETACHED) {
>>>> @@ -530,16 +533,21 @@ void qemu_thread_create(QemuThread *thread, const
>>>> char *name,
>>>> qemu_thread_args->name = g_strdup(name);
>>>> qemu_thread_args->start_routine = start_routine;
>>>> qemu_thread_args->arg = arg;
>>>> -
>>>> err = pthread_create(&thread->thread, &attr,
>>>> qemu_thread_start, qemu_thread_args);
>>>> -
>>>> - if (err)
>>>> - error_exit(err, __func__);
>>>> + if (err) {
>>>> + error_setg(errp, "pthread_create failed: %s", strerror(err));
>>>> + errno = err;
>>> Same questions here.
>>>
>>>> + pthread_attr_destroy(&attr);
>>>> + g_free(qemu_thread_args->name);
>>>> + g_free(qemu_thread_args);
>>>> + return false;
>>>> + }
>>>>
>>>> pthread_sigmask(SIG_SETMASK, &oldset, NULL);
>>>>
>>>> pthread_attr_destroy(&attr);
>>>> + return true;
>>>> }
>>>>
>>>> void qemu_thread_get_self(QemuThread *thread)
>>>> diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
>>>> index 1a27e1cf6f..96e5d19ca3 100644
>>>> --- a/util/qemu-thread-win32.c
>>>> +++ b/util/qemu-thread-win32.c
>>>> @@ -20,6 +20,7 @@
>>>> #include "qemu/thread.h"
>>>> #include "qemu/notify.h"
>>>> #include "qemu-thread-common.h"
>>>> +#include "qapi/error.h"
>>>> #include <process.h>
>>>>
>>>> static bool name_threads;
>>>> @@ -388,9 +389,9 @@ void *qemu_thread_join(QemuThread *thread)
>>>> return ret;
>>>> }
>>>>
>>>> -void qemu_thread_create(QemuThread *thread, const char *name,
>>>> - void *(*start_routine)(void *),
>>>> - void *arg, int mode)
>>>> +bool qemu_thread_create(QemuThread *thread, const char *name,
>>>> + void *(*start_routine)(void *),
>>>> + void *arg, int mode, Error **errp)
>>>> {
>>>> HANDLE hThread;
>>>> struct QemuThreadData *data;
>>>> @@ -409,10 +410,17 @@ void qemu_thread_create(QemuThread *thread, const
>>>> char *name,
>>>> hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
>>>> data, 0, &thread->tid);
>>>> if (!hThread) {
>>>> - error_exit(GetLastError(), __func__);
>>>> + if (data->mode != QEMU_THREAD_DETACHED) {
>>>> + DeleteCriticalSection(&data->cs);
>>>> + }
>>>> + error_setg_win32(errp, GetLastError(),
>>>> + "failed to create win32_start_routine");
>>>> + g_free(data);
>>>> + return false;
>>>> }
>>>> CloseHandle(hThread);
>>>> thread->data = data;
>>>> + return true;
>>>> }
>>>>
>>>> void qemu_thread_get_self(QemuThread *thread)
>>> Fam
>>>
>>>
>>>
>
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH RFC v4 7/7] qemu_thread_create: propagate theerror to callers to handle,
Fei Li <=