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

[Peter Xu <peterx@redhat.com>]

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.

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.

Regards,

-- 
Peter Xu