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

[David Gibson <dgibson@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2880 bytes --]

On Thu, 13 Dec 2018 08:26:48 +0100
Markus Armbruster <armbru@redhat.com> 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.
> 
> Still missing from the commit message then: how you update the callers.
> Let's see below.

[snip]
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -478,6 +478,7 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu,
> >      sPAPRPendingHPT *pending = spapr->pending_hpt;
> >      uint64_t current_ram_size;
> >      int rc;
> > +    Error *local_err = NULL;
> >  
> >      if (spapr->resize_hpt == SPAPR_RESIZE_HPT_DISABLED) {
> >          return H_AUTHORITY;
> > @@ -538,8 +539,13 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu,
> >      pending->shift = shift;
> >      pending->ret = H_HARDWARE;
> >  
> > -    qemu_thread_create(&pending->thread, "sPAPR HPT prepare",
> > -                       hpt_prepare_thread, pending, QEMU_THREAD_DETACHED);
> > +    if (!qemu_thread_create(&pending->thread, "sPAPR HPT prepare",
> > +                            hpt_prepare_thread, pending,
> > +                            QEMU_THREAD_DETACHED, &local_err)) {
> > +        error_reportf_err(local_err, "failed to create hpt_prepare_thread: ");
> > +        g_free(pending);
> > +        return H_RESOURCE;
> > +    }
> >  
> >      spapr->pending_hpt = pending;
> >    
> 
> This is a caller that returns an error code on failure.  You change it
> to report the error, then return failure.  The return failure part looks
> fine.  Whether reporting the error is appropriate I can't say for sure.
> No other failure mode reports anything.  David, what do you think?

I think it's reasonable here.  In this context error returns and
reported errors are for different audiences.  The error returns are for
the guest, the reported errors are for the guest administrator or
management layers.  This particularly failure is essentially a host
side fault that is mostly relevant to the VM management.  We have to
say *something* to the guest to explain that the action couldn't go
forward and H_RESOURCE makes as much sense as anything.

-- 
David Gibson <dgibson@redhat.com>
Principal Software Engineer, Virtualization, Red Hat

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]