Re: [Qemu-devel] [PATCH for-4.0 v8 6/7] qemu_thread_create: propagate the error to callers to handle

[Fei Li <fli@suse.com>]

Hi all,

As I am leaving my current company and most reviewers are on holiday,
I'd like to send a new version now:
v9: "qemu_thread: Make qemu_thread_create() handle errors properly",
although some details like whether it is appropriate to report the error
to be seen by the management layer. And I will use my new personal
email address (shirley17fei@gmail.com <mailto:shirley17fei@gmail.com>) 
to follow the new version. :)

Merry Christmas, and have a nice day, thanks all!
Fei

On 12/24/2018 02:53 PM, Fei Li wrote:
>
>
> On 12/24/2018 11:34 AM, Peter Xu wrote:
>> On 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 <fli@suse.com> 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 <fli@suse.com> 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 <armbru@redhat.com>
>>>>>>>> Cc: Daniel P. Berrangé <berrange@redhat.com>
>>>>>>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>>>>>> Signed-off-by: Fei Li <fli@suse.com>
>>>>>>>> ---
>>>>>>>>     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.
> 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 the
> 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..
>> Though 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.
> Thanks for pointing this out. I think my last patch is to fix this 
> problem, that is
> 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,
>>
>
>
>
>