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

[Markus Armbruster]

David Gibson <address@hidden> writes:

> On Thu, 13 Dec 2018 08:26:48 +0100
> Markus Armbruster <address@hidden> 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.
>> 
>> 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.

Double-checking: is it okay to report some failures of this function
(one of two H_RESOURCE failures, to be precise), but not others?