| Message ID | 20180904110822.12863-5-fli@suse.com |
|---|---|
| State | New |
| Headers | show |
| Series | qemu_thread_create: propagate errors to callers to check | expand |
On Tue, Sep 04, 2018 at 07:08:21PM +0800, Fei Li wrote: > Add a new Error paramater for qemu_thread_create() to indicate if it > succeeds rather than failing with an error. And propagate the error > to let the callers check it. > > Besides, directly return if thread->data is NULL to avoid the > segmentation fault in qemu_thread_join in qemu-thread-win32.c. > > Signed-off-by: Fei Li <fli@suse.com> > --- > include/qemu/thread.h | 2 +- > util/qemu-thread-posix.c | 15 +++++++++++---- > util/qemu-thread-win32.c | 12 +++++++++--- > 3 files changed, 21 insertions(+), 8 deletions(-) > > diff --git a/include/qemu/thread.h b/include/qemu/thread.h > index dacebcfff0..71d8be5851 100644 > --- a/include/qemu/thread.h > +++ b/include/qemu/thread.h > @@ -137,7 +137,7 @@ void qemu_event_destroy(QemuEvent *ev); > > void qemu_thread_create(QemuThread *thread, const char *name, > void *(*start_routine)(void *), > - void *arg, int mode); > + void *arg, int mode, Error **errp); You've changed the API signature in this patch, but don't update any of the callers until the next patch. This means that the build will fail on this patch. In order to ensure that "git bisect" can be usable we require that the code is able to build sucessfully on every patch in a series. So I think you'll have to merge patch 5 into this patch to ensure the build succeeds. Regards, Daniel
On 09/04/2018 07:18 PM, Daniel P. Berrangé wrote: > On Tue, Sep 04, 2018 at 07:08:21PM +0800, Fei Li wrote: >> Add a new Error paramater for qemu_thread_create() to indicate if it >> succeeds rather than failing with an error. And propagate the error >> to let the callers check it. >> >> Besides, directly return if thread->data is NULL to avoid the >> segmentation fault in qemu_thread_join in qemu-thread-win32.c. >> >> Signed-off-by: Fei Li <fli@suse.com> >> --- >> include/qemu/thread.h | 2 +- >> util/qemu-thread-posix.c | 15 +++++++++++---- >> util/qemu-thread-win32.c | 12 +++++++++--- >> 3 files changed, 21 insertions(+), 8 deletions(-) >> >> diff --git a/include/qemu/thread.h b/include/qemu/thread.h >> index dacebcfff0..71d8be5851 100644 >> --- a/include/qemu/thread.h >> +++ b/include/qemu/thread.h >> @@ -137,7 +137,7 @@ void qemu_event_destroy(QemuEvent *ev); >> >> void qemu_thread_create(QemuThread *thread, const char *name, >> void *(*start_routine)(void *), >> - void *arg, int mode); >> + void *arg, int mode, Error **errp); > You've changed the API signature in this patch, but don't update any of > the callers until the next patch. This means that the build will fail on > this patch. > > In order to ensure that "git bisect" can be usable we require that the > code is able to build sucessfully on every patch in a series. > > So I think you'll have to merge patch 5 into this patch to ensure the > build succeeds. > > Regards, > Daniel Oops, thanks for the reminder! Will merge them into one in the next version. Have a nice day Fei
diff --git a/include/qemu/thread.h b/include/qemu/thread.h index dacebcfff0..71d8be5851 100644 --- a/include/qemu/thread.h +++ b/include/qemu/thread.h @@ -137,7 +137,7 @@ void qemu_event_destroy(QemuEvent *ev); void qemu_thread_create(QemuThread *thread, const char *name, void *(*start_routine)(void *), - void *arg, int mode); + void *arg, int mode, Error **errp); void *qemu_thread_join(QemuThread *thread); void qemu_thread_get_self(QemuThread *thread); bool qemu_thread_is_self(QemuThread *thread); diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c index dfa66ff2fb..a31c00abfd 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; @@ -506,16 +507,17 @@ static void *qemu_thread_start(void *args) void qemu_thread_create(QemuThread *thread, const char *name, void *(*start_routine)(void*), - void *arg, int mode) + void *arg, int mode, Error **errp) { sigset_t set, oldset; int err; pthread_attr_t attr; QemuThreadArgs *qemu_thread_args; + Error *local_err = NULL; err = pthread_attr_init(&attr); if (err) { - error_exit(err, __func__); + goto fail; } if (mode == QEMU_THREAD_DETACHED) { @@ -534,12 +536,17 @@ void qemu_thread_create(QemuThread *thread, const char *name, err = pthread_create(&thread->thread, &attr, qemu_thread_start, qemu_thread_args); - if (err) - error_exit(err, __func__); + if (err) { + goto fail; + } pthread_sigmask(SIG_SETMASK, &oldset, NULL); pthread_attr_destroy(&attr); + return; +fail: + error_setg(&local_err, "%s", strerror(err)); + error_propagate(errp, local_err); } void qemu_thread_get_self(QemuThread *thread) diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c index 4a363ca675..725200abc9 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; @@ -366,7 +367,7 @@ void *qemu_thread_join(QemuThread *thread) HANDLE handle; data = thread->data; - if (data->mode == QEMU_THREAD_DETACHED) { + if (data == NULL || data->mode == QEMU_THREAD_DETACHED) { return NULL; } @@ -390,10 +391,11 @@ void *qemu_thread_join(QemuThread *thread) void qemu_thread_create(QemuThread *thread, const char *name, void *(*start_routine)(void *), - void *arg, int mode) + void *arg, int mode, Error **errp) { HANDLE hThread; struct QemuThreadData *data; + Error *local_err = NULL; data = g_malloc(sizeof *data); data->start_routine = start_routine; @@ -409,7 +411,11 @@ 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__); + error_setg_win32(&local_err, GetLastError(), + "failed to creat win32_start_routine"); + g_free(data); + error_propagate(errp, local_err); + return; } CloseHandle(hThread); thread->data = data;
Add a new Error paramater for qemu_thread_create() to indicate if it succeeds rather than failing with an error. And propagate the error to let the callers check it. Besides, directly return if thread->data is NULL to avoid the segmentation fault in qemu_thread_join in qemu-thread-win32.c. Signed-off-by: Fei Li <fli@suse.com> --- include/qemu/thread.h | 2 +- util/qemu-thread-posix.c | 15 +++++++++++---- util/qemu-thread-win32.c | 12 +++++++++--- 3 files changed, 21 insertions(+), 8 deletions(-)