|
| From: | Fei Li |
| Subject: | Re: [Qemu-devel] [PATCH for-4.0 v8 6/7] qemu_thread_create: propagate the error to callers to handle |
| Date: | Mon, 24 Dec 2018 14:53:03 +0800 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 |
On 12/24/2018 11:34 AM, Peter Xu wrote:
Thanks for the useful commit. :) I guess the "the client app" mentioned is not qemu, but other upper thing, maybe something inside openstack? As I have to say that I can see the error message (I mean the above error_reportf_err(...) ) be printed to theOn Fri, Dec 21, 2018 at 05:36:57PM +0800, Fei Li wrote:On 12/19/2018 08:14 PM, Fei Li wrote:On 12/19/2018 06:10 PM, Markus Armbruster wrote:Fei Li <address@hidden> writes:On 12/13/2018 03:26 PM, Markus Armbruster wrote:There's a question for David Gibson inline. Please search for /ppc/. Fei Li <address@hidden> writes:Make qemu_thread_create() return a Boolean to indicate if it succeeds rather than failing with an error. And add an Error parameter to hold the error message and let the callers handle it.The "rather than failing with an error" is misleading. Before the patch, we report to stderr and abort(). What about: qemu-thread: Make qemu_thread_create() handle errors properly qemu_thread_create() abort()s on error. Not nice. Give it a return value and an Error ** argument, so it can return success / failure.A nice commit-amend! Thanks!Still missing from the commit message then: how you update the callers.Yes, agree. I think the-how should also be noted here, like - propagating the err to callers whose call trace already have the Error paramater; - just add an &error_abort for qemu_thread_create() and make it a "TODO: xxx";Let's see below.Cc: Markus Armbruster <address@hidden> Cc: Daniel P. Berrangé <address@hidden> Cc: Dr. David Alan Gilbert <address@hidden> Signed-off-by: Fei Li <address@hidden> --- cpus.c | 45 ++++++++++++++++++++++++------------- dump.c | 6 +++-- hw/misc/edu.c | 6 +++-- hw/ppc/spapr_hcall.c | 10 +++++++-- hw/rdma/rdma_backend.c | 4 +++- hw/usb/ccid-card-emulated.c | 16 ++++++++++---- include/qemu/thread.h | 4 ++-- io/task.c | 3 ++- iothread.c | 16 +++++++++----- migration/migration.c | 54 +++++++++++++++++++++++++++++---------------- migration/postcopy-ram.c | 14 ++++++++++-- migration/ram.c | 40 ++++++++++++++++++++++++--------- migration/savevm.c | 11 ++++++--- tests/atomic_add-bench.c | 3 ++- tests/iothread.c | 2 +- tests/qht-bench.c | 3 ++- tests/rcutorture.c | 3 ++- tests/test-aio.c | 2 +- tests/test-rcu-list.c | 3 ++- ui/vnc-jobs.c | 17 +++++++++----- ui/vnc-jobs.h | 2 +- ui/vnc.c | 4 +++- util/compatfd.c | 12 ++++++++-- util/oslib-posix.c | 17 ++++++++++---- util/qemu-thread-posix.c | 24 +++++++++++++------- util/qemu-thread-win32.c | 16 ++++++++++---- util/rcu.c | 3 ++- util/thread-pool.c | 4 +++- 28 files changed, 243 insertions(+), 101 deletions(-)...snip, and only leave the three uncertain small topics...diff --git a/migration/ram.c b/migration/ram.c index 658dfa88a3..6e0cccf066 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -473,6 +473,7 @@ static void compress_threads_save_cleanup(void) static int compress_threads_save_setup(void) { int i, thread_count; + Error *local_err = NULL; if (!migrate_use_compression()) { return 0; @@ -502,9 +503,12 @@ static int compress_threads_save_setup(void) comp_param[i].quit = false; qemu_mutex_init(&comp_param[i].mutex); qemu_cond_init(&comp_param[i].cond); - qemu_thread_create(compress_threads + i, "compress", - do_data_compress, comp_param + i, - QEMU_THREAD_JOINABLE); + if (!qemu_thread_create(compress_threads + i, "compress", + do_data_compress, comp_param + i, + QEMU_THREAD_JOINABLE, &local_err)) { + error_reportf_err(local_err, "failed to create do_data_compress: "); + goto exit;[1]+ } } return 0;Reviewing the migration changes is getting tiresome...Yes, indeed, the migration involves a lot! Thanks so much for helping to review!Is reporting the error appropriate here, and why?I think the qemu monitor should display the obvious and exact failing reason for administrators, esp considering that qemu_thread_create() itself does not print any message thus we have no idea which direct function fails if gdb is not enabled. IOW, I think David's answer to that ppc's error_reportf_err() also apply here: "The error returns are for the guest, the reported errors are for the guest administrator or management layers."There could well be an issue with the "management layers" part. Should this error be sent to the management layer via QMP somehow? Migration maintainers should be able to assist with this question.Kindly ping migration maintainers. :)I think both the maintainers are on holiday so possibly there won't be any reply from them this week... :) Regarding to error reports of migration via QMP layer, please have a look at d59ce6f344 ("migration: add reporting of errors for outgoing migration", 2016-05-26). Though I see that even qemu_savevm_state_setup() is not capturing error for the management layer so if you want to pass this thread creation error upward you'll possibly need to work on that as well.
screen when I use qemu command line via hmp to do the migration.For the qemu_savevm_state_setup(), I see it sets the f->last_error (instead of s->error) to indicate whether to stop the migration or not when back to migration_thread() in migration_detect_error(s). And no matter whether qemu_savevm_state_setup() succeeds, the current code continues to set the migration state to be ACTIVE. Emm,
I am wondering whether this is on purpose..
Thanks for pointing this out. I think my last patch is to fix this problem, that isThough here note that when you "goto exit" at [1] you probably also need to touch up the cleanup part since otherwise the join() could be with an invalid thread ID, so you'll possibly need to check the thread ID validity before do the join() of the compression thread.
to add a check in qemu_thread_join():
+ if (!thread->thread) {
+ return NULL;
+ }
Correct me if this is not the proper solution. :)
Have a nice day, thanks :)
Fei
Regards,
| [Prev in Thread] | Current Thread | [Next in Thread] |